# Review: live routing-decisions panel (`events.py`, `tui_sse.py`, SSE endpoint)

Status: done -- review of shipped work

**Scope.** Commits `f0ebd83` ("feat(tui): live routing-decisions panel with
SSE, detail popup, breakdown") and `765a4a6` ("docs: sweep for live routing
panel + AGENTS.md"), i.e. everything since the last-reviewed commit
`5f7716e`. 13 files, +1188/-130: new `events.py` (in-memory decision
broker), new `tui_sse.py` (background SSE consumer thread), a new
`GET /events/decisions` endpoint in `dispatcher.py`, and the corresponding
`tui.py`/`tui_model.py`/`tui_screens.py` wiring. Full suite: 562/562
passing.

Run via `/code-review high --since 5f7716e` (forked, finder-angle + verify
phases), then independently re-derived and confirmed against the actual
files rather than trusted as-is — every finding below was re-read against
the committed source at the cited file/line before being included.

## Findings

### 1. `events.py`'s subscriber set is mutated from multiple threads with no lock — `events.py:24-41,61,67`

FastAPI runs its sync `def` endpoints (`/dispatch`, `/route`,
`/v1/chat/completions`, and the new `/events/decisions`) in a thread pool.
`_subscribers` is a plain `set()`. `publish_decision` iterates it directly
(`for subscriber in _subscribers:`, line 36) while `subscribe()`/`unsubscribe()`
mutate it with `.add()`/`.discard()` from whatever thread is serving a
concurrent dashboard connect/disconnect. A decision recorded at the same
instant a dashboard connects or drops raises
`RuntimeError: Set changed size during iteration`. Confirmed by reading —
this is a plain, unguarded `set`, no `threading.Lock` anywhere in the file.

Where it lands matters: raised inside `persist_route_decision` it's caught
by that function's broad `except Exception` and silently swallowed — the
live fan-out for that one decision is just dropped, no crash, no log.
Raised inside `events.subscribe()` at the top of `_decision_event_stream`
(dispatcher.py:1312), before that generator's own `try/finally`, it's
unhandled and can break a new dashboard connection outright.

Directly contradicts the module's own docstring
("thread-safe fan-out to SSE subscribers" — `CLAUDE.md`, and `events.py:1-12`
describes the same intent without ever establishing it).

### 2. A subscriber dropped for a full queue is never told, so its SSE stream idles forever — `events.py:39-41` + `dispatcher.py:1317-1323`

When a dashboard falls behind and its 100-slot queue fills, `publish_decision`
silently evicts it from `_subscribers` (no close, no sentinel). The matching
`_decision_event_stream` generator has no idea — it keeps calling
`subscriber.get(timeout=SSE_HEARTBEAT_SECONDS)` on the now-orphaned queue,
which can only ever time out, so it emits `:heartbeat` forever
(dispatcher.py:1317-1323). The HTTP connection never errors, so
`tui_sse.DecisionStream`'s reconnect loop never fires. The dashboard looks
alive — table renders, connection stays open — but silently stops receiving
any new decision until the process is restarted. Confirmed by reading both
sides of the queue handoff.

### 3. Live decisions are appended with no id-based dedup, so every reconnect (and the very first connect) duplicates rows already shown — `tui.py:234-249`

`_decision_event_stream` always replays the ring buffer on connect
(`events.subscribe(replay=True)`, dispatcher.py:1312), and
`tui_sse.DecisionStream.run()` reconnects automatically 5s after any
transient error (`tui_sse.py:45-61`). `_handle_live_decision` (tui.py:238)
unconditionally `insert(0, ...)`s every decision it receives into
`recent_decisions` with no check against ids already present. Since the
initial `/metrics` poll on startup already populates the same recent
decisions, the very first SSE replay duplicates them immediately; every
later reconnect duplicates again. `build_category_breakdown` runs over this
same list (line 246), so the per-category counts and "majority model" in
the breakdown panel skew from replay noise, not real traffic. Confirmed by
reading — no `id` set or seen-check anywhere in `_handle_live_decision` or
`decision_row`.

### 4. `tui_sse.py`'s reconnect loop only catches network/parse errors, logs nothing, and can be permanently killed by its own callback — `tui_sse.py:45-61`

`self.callback(...)` (line 56) is `App.call_from_thread`, which can raise
once the Textual app's event loop is gone — e.g. during shutdown, since
`stop()` (line 63) only sets an `Event` and does not interrupt a blocking
`iter_lines()` read, so the thread can still be mid-callback for up to
`STREAM_TIMEOUT` seconds after `on_unmount` calls `stop()`. That's not a
`requests.RequestException` or `ValueError`, so it isn't caught by the
`except` on line 57 — it propagates out of `run()` and ends the thread for
good, no further reconnect attempts, ever.

Separately, even the errors that *are* caught are swallowed with a bare
`pass` (line 60) — no `logs.warning(...)` call, unlike the identical
"this must never raise" pattern used elsewhere in this codebase (e.g.
`persist_route_decision`'s except block, which does log). An ordinary
dropped VPN tunnel or router restart produces zero diagnostic trace here.

### 5. `/events/decisions` is a sync endpoint that blocks in the shared threadpool for the life of each SSE connection — `dispatcher.py:1328-1329`

Confirmed: `def events_decisions():`, not `async def`. FastAPI runs sync
endpoints in its bounded default threadpool, the same pool serving
`/dispatch`, `/route`, and `/v1/chat/completions`. A handful of connected
dashboards, or `DecisionStream`'s reconnect loop flapping through repeated
transient failures (finding #4 makes that worse — a dead thread means a
`textual` restart reconnects from scratch, briefly doubling in-flight
connections), can hold enough concurrent long-lived streams to exhaust the
pool and stall real routing/dispatch requests behind idle SSE connections.

### 6. `DecisionDetailScreen.on_key` risked a double-dismiss on Enter — `tui_screens.py:65` (as committed in `765a4a6`)

As committed: `if event.key in ("escape", "q", "enter"): self.dismiss(None)`
with no `event.stop()`. When the Close button has focus and Enter is
pressed, Textual delivers the key to the focused `Button` first; `Button`
has no key handler of its own, so the event bubbles unstopped to this
`on_key`, which dismisses — but the event can *also* continue bubbling to
the App's binding resolution and match `Button.BINDINGS`'s own `enter`
binding, firing `action_press()` → `Button.Pressed` →
`on_button_pressed` → a second `self.dismiss(None)` on an already-popped
modal. Confirmed against the exact committed line via `git show 765a4a6`.

**Already independently fixed in the working tree, uncommitted, as of this
review** — `git diff -- tui_screens.py` shows `on_key` narrowed to
`if event.key == "escape":` only, dropping `"enter"`/`"q"` handling from
this method entirely (Enter now only ever reaches `Button`'s own binding,
`q`'s docstring/label mention removed too). This resolves the race by
construction rather than by adding `event.stop()`. No action needed here —
noting it so the fix isn't lost if the working tree changes again before
it's committed.

### 7. Two independent `datetime.now()` calls give the SSE payload a different `observed_at` than the persisted row for the same decision — `dispatcher.py:923` vs `dispatcher.py:954`

The `INSERT` is stamped at line 923; `events.publish_decision(...)`'s
payload is stamped by a second, separate call at line 954, several
statements later and after `conn.commit()`. A dashboard that correlates the
live SSE event for decision `id=N` against the same row fetched later via
`/metrics` sees `observed_at` differ by the insert/commit latency — small in
practice, but it breaks the assumption (implicit in the code's own comment
at line 949-950, "the row id becomes the ordering handle") that the SSE
payload mirrors the persisted row exactly. Trivial fix: reuse one timestamp
for both.

### 8. `_decision_event_stream` hand-rolls SSE frame construction as a third, inconsistent implementation — `dispatcher.py:1303-1323`

`dispatcher.py` already builds `text/event-stream` frames twice elsewhere
(the OpenAI-compatible streaming wrapper and the real streaming proxy),
both via `yield f"data: {json.dumps(...)}\n\n".encode()`. The new generator
reimplements the same primitive and is the only one of the three that
*doesn't* `.encode()` the yielded string (relying on Starlette's
`StreamingResponse` to encode `str` chunks for it, which does work — this
isn't a bug — but it's now three copies of one pattern that can silently
drift apart on the next edit to any one of them). Minor; a shared helper
would remove the inconsistency rather than fix a defect.

### 9. Every live decision triggers a full sort + `Counter` rebuild over the whole recent-decisions list — `tui_model.py` (`build_category_breakdown`) via `tui.py:246-248`

`_handle_live_decision` calls `build_category_breakdown` over the entire
(up to 50-item) list on every single SSE event, doing a full sort and
`Counter` rebuild on the Textual UI thread each time, when only the one
`(category, tier)` bucket the new decision falls into actually changed.
Not a correctness bug — under realistic traffic volumes (a handful of
decisions/sec at most) this is imperceptible — but under a burst it's
doing O(n log n) work per event for an O(1) update, on the UI thread.
Lowest priority of the nine; noted for completeness rather than urgency.

## What's solid

- The core SSE plumbing works end-to-end: replay-then-live, heartbeats,
  `retry:` hint, and the `unsubscribe`-in-`finally` shutdown path are all
  correctly shaped for the common case (one dashboard, no reconnect storms).
- `/events/decisions`'s docstring is accurate about what it does and doesn't
  carry (no conversation text, prompt, or `session_dir` — matches the same
  privacy invariant already enforced for `route_decisions` rows).
- `_drain_queue`'s non-blocking drain-then-block pattern (dispatcher.py:1293-1300,
  1315-1323) is the right shape for "flush anything buffered, then wait" and
  is itself correct in isolation.
- 562/562 tests pass, including new coverage in `test_events.py`,
  `test_metrics_endpoint.py`, and `test_tui.py` for the parts of this
  feature that are correct.

## Recommendation

Findings #1 and #2 are the ones worth fixing before this sees real
multi-dashboard or flaky-network use — #1 is a live `RuntimeError` under
ordinary concurrent access (not a rare race window; any connect/disconnect
overlapping a `publish_decision` call triggers it), and #2 silently breaks
the exact feature this commit exists to ship. #4 (dead reconnect thread) and
#3 (duplicate rows on every reconnect) compound #1/#2 — a dashboard that hit
the `RuntimeError` and then can't reconnect because its thread died, showing
stale-but-plausible data with no error, is a bad failure mode for something
meant to be watched passively. #5, #7, #8, #9 are all real but low-severity
and can ride along with the same pass. #6 is already resolved in the
uncommitted working tree.