6krrt/plans/resolve-review-findings-sse-lock-gap-review.md

# Review: re-check of `resolve-review-findings` lift (commits `e24006a`, `32e1c30`)

Status: done -- review of shipped work

**What it was reviewing:** opencode/Atlas's "resolve-review-findings" orchestration,
which claimed to resolve three findings from
[`session-cache-and-baseline-comparator-review.md`](session-cache-and-baseline-comparator-review.md),
[`tui-live-routing-panel-sse-fix-review.md`](tui-live-routing-panel-sse-fix-review.md),
and [`flex-preference-knob-review.md`](flex-preference-knob-review.md), with a
Final Wave of F1-F4 all `[APPROVE]`/`[PASS]`. Spot-checked every claim against
the actual diff rather than the delivery summary. Full suite: 634/634 passing.

## Verdict: 3 of 4 claims hold; the SSE lock gap does not

### Baseline report filter fix — correct, matches the review exactly

`baseline_report.py`'s `load_decisions` now filters
`"kind IN ('route', 'chat', 'dispatch')"` and adds
`"selected_model IS NOT NULL"` — exactly the two clauses
`session-cache-and-baseline-comparator-review.md` asked for. Live check:
`/health`-adjacent `route_decisions` table now yields the real dogfooding
volume instead of the 10-row `kind='route'` sliver. No further action.

### Flex-preference sibling re-gate — correct, was already in HEAD as claimed

`routing.py:267`'s `apply_flex_preference` calls `rejection_reason(sibling,
..., latency_tolerance=BATCH, ...)` before accepting a flex swap, matching
`flex-preference-knob-review.md`'s recommendation. Confirmed the 4 sibling
tests (stale/canary x prefer-flex/force-flex) exist and pass. No further
action.

### SSE loop-capture bug (finding #5) — correctly fixed

`events.subscribe_sse` now captures `asyncio.get_running_loop()` at
subscribe time (called from the `async def events_decisions()` generator,
where it's valid) and stores it in `_sse_loops`, so `publish_decision`
bridges via the *subscriber's* loop rather than calling
`asyncio.get_event_loop()` from the publisher's thread. This is the actual
fix the prior review asked for, and it's the right mechanism. The bounded
queue (`asyncio.Queue(maxsize=events.DEFAULT_QUEUE_SIZE)` at
`dispatcher.py:1381`) and drop-oldest policy (`events._sse_push`) are both
present and match the recommendation too. Dead code (`subscribe`/`unsubscribe`/
`_EVICTED`/`import queue`) is gone, the `queue` stdlib shadowing is resolved
(parameters renamed to `sse_queue`/`decision_queue`), and the dropped
docstring line ("no conversation text, prompt, or session_dir") is restored
at `dispatcher.py:1405-1407`. All of this matches the delivery summary.

### Secondary issue #2 from the same review — NOT fixed, and the new code's own comment says it is

**File:** `events.py:54-73`, `publish_decision`.

**Problem.** The prior review's secondary issue #2 was: "`_sse_subscribers`
iterated without the lock that guards its mutation ... A connect/disconnect
racing a publish can raise `RuntimeError: Set changed size during
iteration`." The rewrite moved from a `set` to a `dict` (`_sse_loops`) but
carried the same gap forward rather than closing it:

```python
    # Bridge to async SSE subscribers on their own loops. Iterate under
    # the lock so a connect/disconnect cannot race this into a
    # "Set changed size during iteration" error. call_soon_threadsafe is
    # thread-safe, so this is safe to call from any thread.
    for sse_queue, loop in list(_sse_loops.items()):
        try:
            loop.call_soon_threadsafe(_sse_push, sse_queue, decision)
        except RuntimeError:
            _sse_subscribers.discard(sse_queue)
            _sse_loops.pop(sse_queue, None)
```

The comment says "Iterate under the lock." There is no `with
_subscribers_lock:` anywhere in this function. `subscribe_sse` (line 90),
`unsubscribe_sse` (line 103), and `clear` (line 119) all take
`_subscribers_lock` before touching `_sse_subscribers`/`_sse_loops`;
`publish_decision` — called from arbitrary anyio worker threads on every
routing decision — does not, either for the read (`list(_sse_loops.items())`)
or for the mutation in the `except` branch (`.discard()` / `.pop()`).

**Severity, checked empirically rather than assumed.** Reproduced with a
direct stress test: one thread calling `publish_decision` in a tight loop
while the event loop thread repeatedly subscribes/unsubscribes (churning
`_sse_loops`), ~200k iterations. Zero `RuntimeError`s. On standard
GIL-enabled CPython, `list(dict.items())` for a dict keyed by
identity-hashed objects (`asyncio.Queue`) runs as a single C-level
operation that doesn't yield the GIL mid-iteration, so the exact failure
mode the comment describes is very unlikely to fire in practice — this is
not the live, silently-broken-in-production class of bug #5 was. But it is
a real gap: the invariant the lock exists to provide isn't actually
provided by this function, the comment asserts something false about the
code next to it, and this project's Python version matrix (`tests/`
"Verified on Python 3.10 and 3.14") doesn't rule out a free-threaded
(`--disable-gil`) build, where this would be a genuine data race rather
than a theoretical one.

**Why the tests don't catch it:** `tests/test_events.py` has no test that
references `Lock`, `lock`, `_sse_loops`, or `_subscribers_lock` at all —
confirmed by grep. The new regression tests added this round cover the
loop-capture fix and queue bounding/drop-oldest, not lock safety. This is
the same shape as the original miss: a fixture/test scope that doesn't
exercise the concurrent path the comment is making a claim about.

**Fix.** Wrap the iteration and the `except` branch's mutation in
`with _subscribers_lock:`, matching every other accessor of these two
collections:

```python
    with _subscribers_lock:
        subscribers = list(_sse_loops.items())
    for sse_queue, loop in subscribers:
        try:
            loop.call_soon_threadsafe(_sse_push, sse_queue, decision)
        except RuntimeError:
            with _subscribers_lock:
                _sse_subscribers.discard(sse_queue)
                _sse_loops.pop(sse_queue, None)
```

Then add a test that actually exercises concurrent subscribe/unsubscribe
against a publishing thread (the shape used to reproduce this above), so a
regression here isn't silent a third time.

## Recommendation

Everything else in this lift is solid and needs no rework. Close this one
gap in `events.py::publish_decision` — one `with` block, mirroring the
other three accessors in the same file — and add the concurrent-churn test
described above.