mirror of
https://github.com/turnstonelabs/turnstone.git
synced 2026-08-20 11:00:59 -06:00
3abd2c441b
* feat(console): coord rich ws_state payload + live activity broadcast (Stage 2 follow-up)
Pre-lift coord's cluster broadcast was state-only — the dashboard's
coord rows showed the state column flipping but ``tokens`` /
``context_ratio`` / ``activity`` / ``content`` were all hardcoded
to zero / empty. The lift makes coord populate the same per-ws
metric fields interactive does and broadcasts them through the
cluster collector with the rich kwargs.
**Architecture changes:**
- Lift ``on_status`` / ``on_content_token`` / ``on_thinking_start`` /
``on_thinking_stop`` / ``on_stream_end`` / ``on_tool_result`` /
``on_reasoning_token`` / ``on_tool_output_chunk`` / ``on_info`` /
``on_error`` from ``WebUI`` to :class:`SessionUIBase` as base
implementations. Coord inherits the bodies; the per-ws metric
fields it had at the base but never populated now flow.
- ``WebUI`` keeps overrides for ``on_status`` / ``on_tool_result`` /
``on_error`` to layer Prometheus ``_metrics.record_*`` calls
on top of ``super()`` (node-only — the console isn't a node).
``WebUI._broadcast_state`` now uses the new
:meth:`SessionUIBase.snapshot_and_consume_state_payload` helper
for the rich-payload snapshot read.
- ``ConsoleCoordinatorUI`` adds a ``_broadcast_activity`` override
that calls the new
:meth:`ClusterCollector.update_console_ws_activity` (in-memory
pseudo-node row update; named ``update_*`` rather than ``emit_*``
to flag the no-fanout asymmetry vs. the rest of the
``emit_console_ws_*`` family).
- ``coord_adapter.emit_state`` reads ``ws.ui``'s snapshot under
``_ws_lock`` and passes the rich kwargs to the extended
:meth:`ClusterCollector.emit_console_ws_state`. Defensive when
``ws.ui is None`` mid-eviction (broadcasts state-only).
- ``coord_endpoint_config`` wires a new ``_coord_spawn_metrics``
hook so per-spawn ``_ws_messages`` / ``_ws_turn_tool_calls``
bookkeeping fires on coord too.
- ``_MAX_TURN_CONTENT_CHARS`` moved from ``turnstone.server`` to
``turnstone.core.session_ui_base`` so coord enforces the same
per-turn content cap.
**Three observable behaviour changes** (CHANGELOG-callout-worthy):
- Coord persists ``usage_event`` storage rows on every status
emission (governance dashboards / token-spend queries gain
coord visibility).
- Coord broadcasts live activity transitions to the cluster
collector (dashboard's coord rows show activity ticks between
state changes the same way interactive does), with last-emitted
dedup so a tool-heavy turn's repeated ``activity=""`` clears
don't hammer the collector lock.
- Cluster ``cluster_state`` events for coord rows now carry
non-zero ``tokens`` / ``content``. Frontend rendering that
conditionally hid these on coord can drop the branch.
**Tests:** 23 new tests in ``tests/test_coord_rich_ws_state_payload.py``
(per-ws metric writes, snapshot helper drain semantics +
single-lock-acquisition, adapter rich-payload pass-through +
None-UI defensive handling, activity broadcast wire + dedup +
failure swallow + no-op-when-collector-unset, spawn_metrics
hook, concurrent-writes-during-snapshot stress with reader
cycling through running/idle/error so drain branches actually
run, on_stream_end activity-clear pin). Plus WebUI override
regression tests confirming ``_metrics.record_*`` still fires
on top of the lifted bodies. Existing
``tests/test_webui_content.py`` updated to import
``_MAX_TURN_CONTENT_CHARS`` from its new home;
``tests/test_coordinator_adapter.py`` updated to expect the
rich-payload kwargs (default zeros) on
``emit_console_ws_state``. Total: ``4491 → 4514``.
``ruff check`` clean, ``mypy`` clean on touched files.
**/review pipeline** (4 finders → verify → dedupe) caught 14
findings → 12 unique (3 collapsed as duplicates of the lockless
``on_content_token`` writer):
- bug-1 Minor: ``on_status`` regressed coord's defensive
``usage.get(...)`` indexing → restored ``.get(..., 0)`` for
``prompt_tokens`` / ``completion_tokens`` on both base + WebUI
override.
- bug-2 Nit: concurrent-snapshot reader only used ``"running"`` →
cycled through ``("running", "idle", "error")`` so drain
branches run; also captures + re-raises thread exceptions
instead of silently passing.
- bug-3 + sec-2 + perf-3 Nit (merged): ``on_content_token``
mutated ``_ws_turn_content`` lockless while the snapshot drained
under lock → wrapped the cap-check + append + size-update in
``_ws_lock``.
- perf-2 Minor: collector lock contention from per-event activity
broadcasts → cached last-emitted ``(activity, activity_state)``
on the UI; subsequent identical ticks return early without
acquiring the collector lock.
- perf-4 Nit: join-under-lock in snapshot helper → swap-then-join
pattern (capture list reference under lock, reassign to empty,
join the captured list outside the lock). Halves the lock
hold and decouples the join walk from concurrent appenders.
- q-1 Minor: ``emit_console_ws_activity`` was misleading (no
``_fanout`` call, unlike the rest of the ``emit_console_ws_*``
family) → renamed to ``update_console_ws_activity`` + docstring
call-out for the asymmetry.
- q-2 + q-3 Minor/Nit: stale docstrings on
``coordinator_ui.py`` (still claimed "no per-node metrics —
Phase D") and ``_interactive_spawn_metrics`` (still claimed
"counters live on WebUI only") → both updated to reflect the
lifted base class + coord's new hook.
- q-4 Nit: broken Sphinx cross-ref
``:meth:\`_snapshot_and_consume_state_payload\``` → dropped
the leading underscore.
- q-5 Nit: missing ``test_coord_on_stream_end_clears_activity``
→ added.
**Two findings explicitly deferred** (out-of-scope follow-ups,
documented in CHANGELOG):
- perf-1: synchronous ``record_usage_event`` INSERT on coord
worker thread per status tick. Parity with WebUI is the lift's
goal; if throughput becomes a concern, batch usage_event writes
on a background flusher (would apply to both kinds).
- sec-1: coord assistant content now flows on the cluster SSE
stream, which has no per-user filter today. Pre-existing
exposure for interactive ``cluster_state`` events; the lift
extends to coord rows. Proper fix needs SSE auth gating
(``admin.cluster.inspect``) or per-listener user_id filtering
— separate security project, doesn't gate this lift.
* fix(console): apply review feedback on PR #420
Three review findings, all confirmed against source:
1. **Copilot — dedup-state-vs-failure race in `_broadcast_activity`**
(correctness bug): pre-fix ``self._last_broadcast_activity = current``
was assigned inside the ``_ws_lock`` block BEFORE the collector call.
If the collector raised mid-broadcast, the exception was swallowed
but the dedup state was already updated, so subsequent identical
activity ticks would be deduped and never retried — leaving the
dashboard's coord row stranded at the pre-failure activity until
the activity actually changed.
Fix: move the dedup-state update OUT of the lock and place it AFTER
a successful collector call. On failure, ``_last_broadcast_activity``
stays unchanged so the next identical tick retries. Two new
regression tests pin both the failure-recovery (``test_coord_ui_
broadcast_activity_failure_does_not_strand_dedup``) and the
happy-path dedup behavior (``test_coord_ui_broadcast_activity_
dedup_skips_identical_after_success``).
2. **Copilot — stale `emit_console_ws_activity` reference in
CHANGELOG**: the method was renamed to ``update_console_ws_activity``
per /review's q-1 finding before the original commit landed, but the
CHANGELOG entry was written ahead of the rename. Updated to match
the actual API + added the no-fanout asymmetry rationale inline so
readers don't have to chase the method name.
3. **code-quality bot ×2 — `except BaseException` in test workers**:
the concurrent-snapshot stress test caught thread-worker exceptions
with ``except BaseException`` (with a noqa to suppress BLE001).
``BaseException`` is overkill for a thread worker — ``SystemExit``
/ ``KeyboardInterrupt`` are main-thread signals and ``Exception``
is the right scope. Narrowed to ``except Exception`` on both
workers; ``writer_exc`` / ``reader_exc`` types narrowed from
``list[BaseException]`` to ``list[Exception]``.
Tests: ``4514 → 4516`` (+2 regression tests for the dedup race fix).
``ruff check`` clean, ``mypy`` clean. No code-path changes outside
the dedup-state placement; the rich-payload broadcast surface is
unchanged.
158 lines
5.7 KiB
Python
158 lines
5.7 KiB
Python
"""Tests for WebUI content accumulation — server-side single source of truth."""
|
|
|
|
import queue
|
|
|
|
import pytest
|
|
|
|
from turnstone.server import WebUI
|
|
|
|
|
|
@pytest.fixture(autouse=True)
|
|
def _reset_global_queue():
|
|
"""Ensure WebUI._global_queue is set for tests and cleaned up after."""
|
|
WebUI._global_queue = queue.Queue()
|
|
yield
|
|
WebUI._global_queue = None
|
|
|
|
|
|
def _make_ui() -> WebUI:
|
|
"""Create a WebUI with a global queue for capturing broadcast events."""
|
|
return WebUI(ws_id="ws-test")
|
|
|
|
|
|
def _drain_global() -> list[dict]:
|
|
"""Drain all events from the global queue."""
|
|
events = []
|
|
assert WebUI._global_queue is not None
|
|
while not WebUI._global_queue.empty():
|
|
events.append(WebUI._global_queue.get_nowait())
|
|
return events
|
|
|
|
|
|
class TestContentAccumulation:
|
|
"""WebUI should accumulate content tokens and include in idle broadcast."""
|
|
|
|
def test_content_token_accumulates(self):
|
|
"""on_content_token should append to _ws_turn_content."""
|
|
ui = _make_ui()
|
|
ui.on_content_token("Hello ")
|
|
ui.on_content_token("world")
|
|
assert ui._ws_turn_content == ["Hello ", "world"]
|
|
|
|
def test_idle_broadcast_includes_content(self):
|
|
"""_broadcast_state('idle') should include joined content and reset."""
|
|
ui = _make_ui()
|
|
ui.on_content_token("Hello ")
|
|
ui.on_content_token("world")
|
|
ui._broadcast_state("idle")
|
|
|
|
events = _drain_global()
|
|
idle_events = [e for e in events if e.get("state") == "idle"]
|
|
assert len(idle_events) == 1
|
|
assert idle_events[0]["content"] == "Hello world"
|
|
# Accumulator should be reset
|
|
assert ui._ws_turn_content == []
|
|
assert ui._ws_turn_content_size == 0
|
|
|
|
def test_error_broadcast_resets_without_content(self):
|
|
"""_broadcast_state('error') should reset accumulator without content in event."""
|
|
ui = _make_ui()
|
|
ui.on_content_token("partial")
|
|
ui._broadcast_state("error")
|
|
|
|
events = _drain_global()
|
|
error_events = [e for e in events if e.get("state") == "error"]
|
|
assert len(error_events) == 1
|
|
assert "content" not in error_events[0]
|
|
assert ui._ws_turn_content == []
|
|
assert ui._ws_turn_content_size == 0
|
|
|
|
def test_thinking_broadcast_does_not_touch_accumulator(self):
|
|
"""_broadcast_state('thinking') should not affect the accumulator."""
|
|
ui = _make_ui()
|
|
ui.on_content_token("in progress")
|
|
ui._broadcast_state("thinking")
|
|
|
|
assert ui._ws_turn_content == ["in progress"]
|
|
events = _drain_global()
|
|
thinking_events = [e for e in events if e.get("state") == "thinking"]
|
|
assert len(thinking_events) == 1
|
|
assert "content" not in thinking_events[0]
|
|
|
|
def test_multi_round_accumulation(self):
|
|
"""Content from multiple streaming rounds accumulates before idle."""
|
|
ui = _make_ui()
|
|
# Round 1
|
|
ui.on_content_token("I'll check ")
|
|
ui.on_content_token("that. ")
|
|
# Round 2 (after tool execution)
|
|
ui.on_content_token("Here's ")
|
|
ui.on_content_token("the result.")
|
|
ui._broadcast_state("idle")
|
|
|
|
events = _drain_global()
|
|
idle_events = [e for e in events if e.get("state") == "idle"]
|
|
assert len(idle_events) == 1
|
|
assert idle_events[0]["content"] == "I'll check that. Here's the result."
|
|
|
|
def test_empty_content_on_idle_without_tokens(self):
|
|
"""idle with no content tokens should include empty content string."""
|
|
ui = _make_ui()
|
|
ui._broadcast_state("idle")
|
|
|
|
events = _drain_global()
|
|
idle_events = [e for e in events if e.get("state") == "idle"]
|
|
assert len(idle_events) == 1
|
|
assert idle_events[0]["content"] == ""
|
|
|
|
def test_cancellation_preserves_partial_content(self):
|
|
"""Partial content accumulated before cancel should appear in idle event."""
|
|
ui = _make_ui()
|
|
ui.on_content_token("I'll ")
|
|
ui.on_content_token("start by...")
|
|
# Cancellation triggers idle broadcast with partial content
|
|
ui._broadcast_state("idle")
|
|
|
|
events = _drain_global()
|
|
idle_events = [e for e in events if e.get("state") == "idle"]
|
|
assert len(idle_events) == 1
|
|
assert idle_events[0]["content"] == "I'll start by..."
|
|
|
|
def test_consecutive_turns_isolated(self):
|
|
"""Content from turn 1 should not leak into turn 2."""
|
|
ui = _make_ui()
|
|
# Turn 1
|
|
ui.on_content_token("first response")
|
|
ui._broadcast_state("idle")
|
|
_drain_global()
|
|
|
|
# Turn 2
|
|
ui.on_content_token("second response")
|
|
ui._broadcast_state("idle")
|
|
|
|
events = _drain_global()
|
|
idle_events = [e for e in events if e.get("state") == "idle"]
|
|
assert len(idle_events) == 1
|
|
assert idle_events[0]["content"] == "second response"
|
|
|
|
def test_content_cap_prevents_unbounded_growth(self):
|
|
"""Content exceeding the cap should stop accumulating."""
|
|
# Constant lifted from turnstone.server to turnstone.core.session_ui_base
|
|
# in the rich ws_state payload work so coord enforces the same ceiling.
|
|
from turnstone.core.session_ui_base import _MAX_TURN_CONTENT_CHARS
|
|
|
|
ui = _make_ui()
|
|
# Fill to capacity
|
|
chunk = "x" * 1024
|
|
for _ in range(_MAX_TURN_CONTENT_CHARS // 1024 + 10):
|
|
ui.on_content_token(chunk)
|
|
|
|
assert ui._ws_turn_content_size <= _MAX_TURN_CONTENT_CHARS + 1024
|
|
ui._broadcast_state("idle")
|
|
|
|
events = _drain_global()
|
|
idle_events = [e for e in events if e.get("state") == "idle"]
|
|
assert len(idle_events) == 1
|
|
# Content should be capped, not contain everything
|
|
assert len(idle_events[0]["content"]) <= _MAX_TURN_CONTENT_CHARS + 1024
|