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, tui-live-routing-panel-sse-fix-review.md, and 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:

    # 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 RuntimeErrors. 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:

    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.