5 follow-up comments from Copilot, all valid:
1. **CRITICAL — snap_seq race with split writer** (concurrency, 001).
Round-1's fix lifted snapshot capture into
register_listener_with_replay under nested locks, but the
WRITER side (on_content_token / on_reasoning_token) still
released _ws_lock before calling _enqueue (which bumps
_event_id under _listeners_lock). A reader could
interleave between writer's release and writer's _enqueue:
capture inflight WITH the new text, read STALE _event_id,
return snap_seq < new_event_id. The new event's live emit
then has _seq > snap_seq, slips past the dedup filter, and
double-renders text the snapshot already contained.
Fix: move self._enqueue(...) INSIDE the with self._ws_lock:
block in both token writers. The inflight mutation and the
_event_id advancement are now atomic against any snapshot
reader. Lock order _ws_lock (outer) → _listeners_lock
(inner via _enqueue) matches the snapshot helpers, so no
deadlock. Fan-out's put_nowait calls happen under
_ws_lock for token writers — microsecond cost per listener,
acceptable for the correctness guarantee.
2. **NIT — stale comment ref to buffered[-1]._event_id** (docs, 002).
The comment referenced a local var (buffered) that lives in
register_listener_with_replay, not in the events handler.
Reworded to describe the cutoff in terms of the last replayed
event id and the atomic-against-writers registration.
3. **MODERATE — 401 branch leaves reconnect loop** (bug, 003).
The coord's onerror schedules a 5 s CLOSED-state recovery timer
unconditionally. In the 401-expired-session branch we close
evtSource and showLogin — but the timer still fires 5 s later,
observes !evtSource, and calls scheduleReconnect(),
which opens a new EventSource that 401s again → infinite
reconnect loop while the login overlay is up. Fix: cancel
reconnectTimer in the 401 branch.
4. **MODERATE — race test was vacuous** (test_coverage, 004).
The previous regression test drained the listener queue after
register_listener_with_replay returned, but the helper
doesn't backfill buffered events into the queue, so the loop
was almost always a no-op and the assertion never executed.
Rewrote with a monkey-patched _enqueue that sleeps 50 ms
before bumping _event_id — widens the race window
deterministically. Verified: the test FAILS on pre-fix code
(snap.content has marker but snap.seq=0 < final_event_id=1)
and PASSES on post-fix code (writer holds _ws_lock through
_enqueue, so the reader blocks until writer fully done).
Also pinned the no-backfill contract so a future change adding
listener-queue backfill remembers to keep snap_seq the
high-water mark.
5. **NIT — except Exception too broad in test** (best_practices, 005).
Tightened except Exception: to except queue.Empty: so
unexpected exceptions aren't silently swallowed in the drain
loop.
Tests:
- 86 tests in test_sse_reconnect_replay.py + test_session_ui_base.py
pass (existing 84 + 2 new race regressions).
- Full non-live suite: 6347 passed, 15 skipped, no regressions.
- Ruff + mypy clean on changed .py files; JS parses.
Patrick Buckley
830305555d