diff --git a/backend/fluksio/api/routes/flows.py b/backend/fluksio/api/routes/flows.py index a9d5d30..63cc189 100644 --- a/backend/fluksio/api/routes/flows.py +++ b/backend/fluksio/api/routes/flows.py @@ -4,6 +4,7 @@ import asyncio import time from typing import Any +import orjson from fastapi import ( APIRouter, Depends, @@ -925,6 +926,22 @@ def event_for_panel(event: dict[str, Any], only: set[str]) -> bool: return event.get("type") == "message_value" and str(event.get("name") or "") in only +# How many events one frame may carry. A ceiling rather than a target: the +# batch is whatever the bus happens to hold, and a client that has been away +# should not be handed the whole queue in one message. +MAX_FRAME_EVENTS = 64 + + +async def _send(websocket: WebSocket, events: list[dict[str, Any]]) -> None: + """One event, or a batch of them under `events`. + + Serialised once with orjson rather than per client with the stdlib, which + is what `send_json` does. + """ + payload = events[0] if len(events) == 1 else {"type": "batch", "events": events} + await websocket.send_text(orjson.dumps(payload).decode()) + + @ws_router.websocket("/ws") async def flow_events(websocket: WebSocket, token: str = "") -> None: """Stream values, node status and execution events as they happen. @@ -932,8 +949,15 @@ async def flow_events(websocket: WebSocket, token: str = "") -> None: The token goes in the query string because browsers cannot set headers on a websocket handshake. """ - with Session(engine) as session: - user = user_from_token(session, token) + + # On a thread: authenticating is a database round trip, and the snapshot + # below reads the whole of state. Neither belongs on the event loop, which + # every other socket and every request is sharing. + def _authenticate() -> Any: + with Session(engine) as session: + return user_from_token(session, token) + + user = await run_in_threadpool(_authenticate) if user is None: await websocket.close(code=1008) return @@ -944,8 +968,13 @@ async def flow_events(websocket: WebSocket, token: str = "") -> None: controller: FlowController | None = getattr( websocket.app.state, "flow_controller", None ) - if controller is not None: - await websocket.send_json(snapshot_payload(controller, only)) + + async def send_snapshot() -> None: + if controller is not None: + payload = await run_in_threadpool(snapshot_payload, controller, only) + await websocket.send_text(orjson.dumps(payload).decode()) + + await send_snapshot() async with event_bus.subscribe() as queue: receiver = asyncio.create_task(websocket.receive_text()) @@ -956,23 +985,47 @@ async def flow_events(websocket: WebSocket, token: str = "") -> None: {sender, receiver}, return_when=asyncio.FIRST_COMPLETED ) if receiver in done: - # The client went away. - sender.cancel() - break - event = sender.result() - if only is not None and event.get("type") == "dashboard_changed": - # The scope was resolved once, at the handshake. A panel - # pointed at another dashboard would otherwise fetch the - # new document and then draw tiles nothing ever updates. - # ``or set()`` because a panel that was deleted resolves to - # None, the same as a person's token — and that would widen - # this socket to everything on the bus. - only = panel_scope(token, websocket.app) or set() - if controller is not None: - await websocket.send_json(snapshot_payload(controller, only)) - if only is not None and not event_for_panel(event, only): + # A frame from the client. Only a disconnect ends the + # stream — a keepalive, or anything else it decides to + # say, used to be read as the client going away and cost + # it every live update from then on. + receiver.exception() + receiver = asyncio.create_task(websocket.receive_text()) + if sender not in done: + continue + elif sender not in done: continue - await websocket.send_json(event) + + # Everything the bus has right now, not just the one event + # that woke this: a cascade puts a dozen in at once, and one + # frame carrying them costs one wakeup rather than a dozen. + batch = [sender.result()] + while len(batch) < MAX_FRAME_EVENTS: + try: + batch.append(queue.get_nowait()) + except asyncio.QueueEmpty: + break + + out: list[dict[str, Any]] = [] + for event in batch: + if only is not None and event.get("type") == "dashboard_changed": + # The scope was resolved once, at the handshake. A + # panel pointed at another dashboard would otherwise + # fetch the new document and then draw tiles nothing + # ever updates. ``or set()`` because a panel that was + # deleted resolves to None, the same as a person's + # token — and that would widen this socket to + # everything on the bus. + only = panel_scope(token, websocket.app) or set() + if out: + await _send(websocket, out) + out = [] + await send_snapshot() + if only is not None and not event_for_panel(event, only): + continue + out.append(event) + if out: + await _send(websocket, out) except WebSocketDisconnect: pass finally: diff --git a/backend/fluksio/flow/events.py b/backend/fluksio/flow/events.py index 80c2755..4acc744 100644 --- a/backend/fluksio/flow/events.py +++ b/backend/fluksio/flow/events.py @@ -9,6 +9,7 @@ from __future__ import annotations import asyncio import logging +import threading from collections import deque from collections.abc import AsyncIterator from contextlib import asynccontextmanager @@ -26,6 +27,13 @@ class EventBus: def __init__(self) -> None: self._loop: asyncio.AbstractEventLoop | None = None self._subscribers: set[asyncio.Queue[dict[str, Any]]] = set() + # Events published since the last flush was scheduled. A cascade + # publishes 13-16 of them and each used to cost its own + # `call_soon_threadsafe`; they now cost one between two turns of the + # loop, which is what a wave is. + self._pending: list[dict[str, Any]] = [] + self._pending_lock = threading.Lock() + self._flush_scheduled = False # Kept whether or not anyone is listening, so opening the log panel # shows what just happened rather than an empty box. self.recent_logs: deque[dict[str, Any]] = deque(maxlen=LOG_HISTORY) @@ -54,21 +62,38 @@ class EventBus: loop = self._loop if loop is None or not self._subscribers: return + with self._pending_lock: + self._pending.append(event) + if self._flush_scheduled: + # A flush is already on its way and has not run yet, so it + # will find this event too. + return + self._flush_scheduled = True try: - loop.call_soon_threadsafe(self._dispatch, event) + loop.call_soon_threadsafe(self._dispatch) except RuntimeError: # Loop already closed — shutting down. - pass + with self._pending_lock: + self._pending.clear() + self._flush_scheduled = False - def _dispatch(self, event: dict[str, Any]) -> None: + def _dispatch(self) -> None: + """Hand everything published since the last turn to every subscriber.""" + with self._pending_lock: + batch = self._pending + self._pending = [] + self._flush_scheduled = False + if not batch: + return for queue in self._subscribers: - if queue.full(): - # Drop the oldest so a slow client never blocks the engine. - try: - queue.get_nowait() - except asyncio.QueueEmpty: - pass - queue.put_nowait(event) + for event in batch: + if queue.full(): + # Drop the oldest so a slow client never blocks the engine. + try: + queue.get_nowait() + except asyncio.QueueEmpty: + pass + queue.put_nowait(event) @asynccontextmanager async def subscribe(self) -> AsyncIterator[asyncio.Queue[dict[str, Any]]]: diff --git a/backend/fluksio/flow/nodes/delay.py b/backend/fluksio/flow/nodes/delay.py index 42871ec..a9d448e 100644 --- a/backend/fluksio/flow/nodes/delay.py +++ b/backend/fluksio/flow/nodes/delay.py @@ -162,9 +162,7 @@ class DelayNode(Node): if self._pipeline is not None and self._pipeline.defer( self, self._to_messages(output) or {}, self.delay ): - logger.debug( - "[%s] Sending %s in %ss", self.name, output, self.delay - ) + logger.debug("[%s] Sending %s in %ss", self.name, output, self.delay) return None time.sleep(self.delay) diff --git a/backend/fluksio/flow/pipeline.py b/backend/fluksio/flow/pipeline.py index b356816..2de189d 100644 --- a/backend/fluksio/flow/pipeline.py +++ b/backend/fluksio/flow/pipeline.py @@ -1725,9 +1725,7 @@ class Pipeline: per timestamp, one at a time under the global state lock. """ keys = [ - k - for k in self._state.keys() - if not k.startswith("__") and (not flow or flow_of(k) == flow) + k for k in self._state.message_names() if not flow or flow_of(k) == flow ] if not keys: return {} diff --git a/backend/fluksio/flow/state.py b/backend/fluksio/flow/state.py index 9875536..67106fe 100644 --- a/backend/fluksio/flow/state.py +++ b/backend/fluksio/flow/state.py @@ -9,7 +9,7 @@ from __future__ import annotations from abc import ABC, abstractmethod from collections import deque -from collections.abc import Iterator +from collections.abc import Iterable, Iterator from contextlib import contextmanager from threading import RLock from typing import Any, cast @@ -98,6 +98,15 @@ class StateBackend(ABC): """Clear all keys in the state.""" ... + def message_names(self) -> list[str]: + """The message names in state, without the bookkeeping keys. + + Its own method because the Redis backend can answer it from a set it + maintains, rather than by scanning a namespace that holds five + bookkeeping keys for every message. + """ + return [k for k in self.keys() if not k.startswith("__")] + def close(self) -> None: # noqa: B027 """Release whatever the backend holds outside this process. @@ -510,18 +519,44 @@ class RedisState(StateBackend): data = cast(bytes | None, self._client.get(self._key(key))) return self._deserialize(data) if data is not None else default + def _names_key(self) -> str: + return self._key("__names__") + + def message_names(self) -> list[str]: + """The message names, from the set kept beside them. + + `keys()` is a SCAN of the whole namespace, which holds a version, a + timestamp, a history list and a last-seen marker for every message — + so reading the message names cost several times as many round trips + as there are messages. Every websocket connect asks for this. + """ + members = cast(set[bytes], self._client.smembers(self._names_key())) + return [m.decode("utf-8") for m in members] + + def _note(self, pipe: Any, keys: Iterable[str]) -> None: + """Record the message names among `keys`, in the caller's pipeline.""" + names = [k for k in keys if not k.startswith("__")] + if names: + pipe.sadd(self._names_key(), *names) + def set(self, key: str, value: Any) -> None: data = self._serialize(value) + pipe = self._client.pipeline() if self._ttl: - self._client.setex(self._key(key), self._ttl, data) + pipe.setex(self._key(key), self._ttl, data) else: - self._client.set(self._key(key), data) + pipe.set(self._key(key), data) + self._note(pipe, [key]) + pipe.execute() def exists(self, key: str) -> bool: return bool(self._client.exists(self._key(key))) def delete(self, key: str) -> None: - self._client.delete(self._key(key), self._history_key(key)) + pipe = self._client.pipeline() + pipe.delete(self._key(key), self._history_key(key)) + pipe.srem(self._names_key(), key) + pipe.execute() def clear(self) -> None: """Clear all keys in the namespace.""" @@ -568,6 +603,7 @@ class RedisState(StateBackend): pipe.setex(self._key(key), self._ttl, data) else: pipe.set(self._key(key), data) + self._note(pipe, mapping) pipe.execute() @contextmanager @@ -780,6 +816,7 @@ class RedisState(StateBackend): pipe.expire(history_key, self._ttl) for key in counters: pipe.incr(self._key(key)) + self._note(pipe, values) pipe.execute() def history(self, key: str) -> list[tuple[float, float]]: diff --git a/backend/tests/flow/test_event_bus.py b/backend/tests/flow/test_event_bus.py new file mode 100644 index 0000000..7db2d17 --- /dev/null +++ b/backend/tests/flow/test_event_bus.py @@ -0,0 +1,83 @@ +"""What the bus costs the event loop, and that it still delivers everything.""" + +import asyncio +import threading + +from fluksio.flow.events import QUEUE_SIZE, EventBus + + +def _event(i: int) -> dict[str, object]: + return {"type": "node_executed", "node": f"n{i}", "outputs": 1} + + +def test_a_cascade_costs_one_loop_callback() -> None: + """The whole point of coalescing. + + A three-node cascade publishes 13-16 events and each used to cross to the + event loop on its own. They are one callback now — and still every event. + """ + + async def run() -> tuple[int, int]: + bus = EventBus() + loop = asyncio.get_running_loop() + bus.bind(loop) + calls = 0 + real = loop.call_soon_threadsafe + + def counting(*args: object, **kwargs: object) -> object: + nonlocal calls + calls += 1 + return real(*args, **kwargs) # type: ignore[arg-type] + + loop.call_soon_threadsafe = counting # type: ignore[method-assign] + try: + async with bus.subscribe() as queue: + # From a worker thread, which is where nodes run. + thread = threading.Thread( + target=lambda: [bus.publish(_event(i)) for i in range(16)] + ) + thread.start() + thread.join() + await asyncio.sleep(0.05) + return calls, queue.qsize() + finally: + loop.call_soon_threadsafe = real # type: ignore[method-assign] + + calls, delivered = asyncio.run(run()) + assert calls == 1 + assert delivered == 16 + + +def test_a_slow_subscriber_still_loses_its_oldest() -> None: + """Coalescing must not turn the drop-oldest bound into unbounded growth.""" + + async def run() -> tuple[int, object]: + bus = EventBus() + bus.bind(asyncio.get_running_loop()) + async with bus.subscribe() as queue: + thread = threading.Thread( + target=lambda: [bus.publish(_event(i)) for i in range(QUEUE_SIZE + 50)] + ) + thread.start() + thread.join() + await asyncio.sleep(0.05) + return queue.qsize(), (await queue.get())["node"] + + size, oldest = asyncio.run(run()) + assert size == QUEUE_SIZE + # The first fifty were dropped, not the last. + assert oldest == "n50" + + +def test_nothing_is_buffered_with_nobody_listening() -> None: + """An engine with no browser open holds no events.""" + + async def run() -> int: + bus = EventBus() + bus.bind(asyncio.get_running_loop()) + for i in range(10): + bus.publish(_event(i)) + await asyncio.sleep(0.01) + return len(bus._pending) + + assert asyncio.run(run()) == 0 diff --git a/backend/tests/flow/test_queue.py b/backend/tests/flow/test_queue.py index a3a8bc2..71cb25e 100644 --- a/backend/tests/flow/test_queue.py +++ b/backend/tests/flow/test_queue.py @@ -4,11 +4,11 @@ import threading import time from fluksio.flow import executor +from fluksio.flow import queue as queue_module from fluksio.flow.executor import ExecutionService from fluksio.flow.messages import DType, MessageSpec from fluksio.flow.nodes import Node from fluksio.flow.pipeline import Pipeline -from fluksio.flow import queue as queue_module from fluksio.flow.queue import MemoryWorkQueue, WorkItem from fluksio.flow.state import MemoryState diff --git a/backend/tests/flow/test_round_trips.py b/backend/tests/flow/test_round_trips.py index b36ba8a..e299ce5 100644 --- a/backend/tests/flow/test_round_trips.py +++ b/backend/tests/flow/test_round_trips.py @@ -68,9 +68,7 @@ def build() -> tuple[Pipeline, Node, CountingState]: requires=[spec("a")], provides=[spec("b")], ) - sink = make_node( - "sink", "chain", lambda b, params: None, requires=[spec("b")] - ) + sink = make_node("sink", "chain", lambda b, params: None, requires=[spec("b")]) state = CountingState() pipeline = Pipeline([source, relay, sink], state=state) return pipeline, source, state diff --git a/frontend/src/components/Flow/liveStore.ts b/frontend/src/components/Flow/liveStore.ts index 52057ae..ce72b10 100644 --- a/frontend/src/components/Flow/liveStore.ts +++ b/frontend/src/components/Flow/liveStore.ts @@ -29,11 +29,6 @@ export type LiveStatus = { /** Why it is queued — what it is waiting for, and on which machine. */ detail?: string | null } -/** How a node's connection is doing, which is not how its last run went. */ -export type NodeHealth = { - health: "ok" | "down" | "unknown" - detail?: string | null -} /** * A node's last failure, kept after it has run again. * @@ -72,7 +67,6 @@ const ENGINE_EVENT_LIMIT = 100 const values = new Map() const statuses = new Map() const failures = new Map() -const health = new Map() let engineEvents: EngineEvent[] = [] // How many times this page has seen a node emit. The number itself means // nothing; a change is what restarts the pulse. @@ -194,13 +188,6 @@ export const liveStore = { path: { name: flow, node_id: node }, }).catch(() => {}) }, - setHealth(nodeId: string, entry: NodeHealth) { - health.set(nodeId, entry) - notify(`health:${nodeId}`) - }, - getHealth(nodeId: string) { - return health.get(nodeId) - }, recordEngineEvent(event: EngineEvent) { // A new array each time, so the hook's snapshot comparison sees the change. engineEvents = [...engineEvents, event].slice(-ENGINE_EVENT_LIMIT) @@ -271,8 +258,6 @@ export const liveStore = { notify(`emit:${key}`) emits.clear() priorEmits.clear() - for (const key of health.keys()) notify(`health:${key}`) - health.clear() engineEvents = [] notify("engine") logLines = [] @@ -304,14 +289,6 @@ export function useNodeFailure(nodeId: string): NodeFailure | undefined { ) } -/** How the node's connection is doing, once it has said anything about it. */ -export function useNodeHealth(nodeId: string): NodeHealth | undefined { - return useSyncExternalStore( - (listener) => subscribeKey(`health:${nodeId}`, listener), - () => health.get(nodeId), - ) -} - /** The last hundred things the engine said about itself, oldest first. */ export function useEngineEvents(): EngineEvent[] { return useSyncExternalStore( diff --git a/frontend/src/components/Flow/useFlowSocket.ts b/frontend/src/components/Flow/useFlowSocket.ts index 41677ea..147eb90 100644 --- a/frontend/src/components/Flow/useFlowSocket.ts +++ b/frontend/src/components/Flow/useFlowSocket.ts @@ -4,6 +4,7 @@ import { useEffect } from "react" import { OpenAPI } from "@/client" import { dashboardKeys, panelKeys } from "@/components/Dashboard/queries" +import { healthKeys } from "@/components/Health/queries" import { runKeys } from "@/components/Runs/queries" import { connectionStore } from "@/lib/connectionStore" import { apiToken } from "@/lib/portal" @@ -146,10 +147,16 @@ const authHandlers = new Set<() => void>() function schedule() { if (timer) return - timer = setTimeout(() => { - timer = null - connect() - }, retry) + // Jittered, because every client of an engine that restarted is counting + // the same backoff from the same moment: without it they all come back + // together, and keep coming back together. + timer = setTimeout( + () => { + timer = null + connect() + }, + retry * (0.5 + Math.random()), + ) retry = Math.min(retry * 2, RECONNECT_MAX) } @@ -164,12 +171,23 @@ function connect() { liveStore.setConnected(true) connectionStore.setSocketOpen(true) // Whatever happened while the socket was down was missed, so nothing - // held in cache can be trusted to still be current. - client?.invalidateQueries() + // held in cache can be trusted to still be current. Scoped to what this + // socket actually feeds: an unqualified invalidation refetches every + // query the page holds, and the usual reason the socket dropped is the + // engine restarting — so every open tab and every wall panel did that at + // once, at the moment it was least able to answer. + for (const queryKey of [ + flowKeys.all, + dashboardKeys.all, + panelKeys.all, + runKeys.all, + healthKeys.all, + ]) { + client?.invalidateQueries({ queryKey }) + } } - ws.onmessage = (event) => { - const message: FlowEvent = JSON.parse(event.data) + const handle = (message: FlowEvent) => { switch (message.type) { case "snapshot": liveStore.setValues(message.values) @@ -216,9 +234,12 @@ function connect() { }) break case "node_health": - liveStore.setHealth(message.node, { - health: message.health, - detail: message.detail, + // The canvas draws node health from the flow detail's `issues`, which + // the server derives — so the screen only moved on mount, navigation + // or a rebuild, never when health actually flipped. The store had a + // health map of its own and nothing ever read it. + client?.invalidateQueries({ + queryKey: message.flow ? flowKeys.detail(message.flow) : flowKeys.all, }) if (message.health === "down") { liveStore.recordEngineEvent({ @@ -292,6 +313,25 @@ function connect() { } } + ws.onmessage = (event) => { + // A frame that is not JSON, or one this bundle cannot read, costs the + // frame rather than the connection: an exception thrown here escapes into + // `window.onerror` and leaves whatever it had already applied behind. + try { + const payload = JSON.parse(event.data) + if (payload?.type === "batch") { + // A cascade publishes a dozen events at once and the engine coalesces + // them into one frame. An installation older than this bundle sends + // them one at a time, which is the branch below. + for (const message of payload.events ?? []) handle(message) + } else { + handle(payload) + } + } catch (error) { + console.warn("Dropped an unreadable socket frame", error) + } + } + ws.onclose = (event) => { // A socket we already dropped: its close says nothing about the connection // we want now.