perf(serial): split daemon WS handler into reader/writer/inbound (closes #749)

[zackees]

Summary

Closes #749. Replaces the daemon's single-loop serial-WebSocket bridge with a producer/consumer/inbound triple-task split so a slow socket flush no longer head-of-line blocks the broadcast drain, eliminating the silent-line-drop behavior FastLED #3219 traced back to here.

What changed

crates/fbuild-daemon/src/handlers/websockets.rs::handle_serial_ws():

• READER task -- broadcast -> mpsc. Always-on, never blocks on socket I/O. Maps Data events to single-line Data messages and forwards non-Data events directly.
• WRITER task -- mpsc -> WS sink. Blocks on first recv().await, then try_recv()-drains everything that arrived during the previous socket flush. Coalesces adjacent Data messages into one Data { lines, current_index }.
• INBOUND task -- WS stream -> serial manager. Handles Write / Detach / ClearBuffer / GetInWaiting. Routes outbound replies (WriteAck, Error, InWaiting) through the same mpsc so WRITER is the sole owner of the WS sink.

Test plan

• Cargo build: soldr cargo build -p fbuild-daemon -> clean
• FastLED autoresearch on Teensy 4.0: bash autoresearch teensy40 --object-fled --strip-sizes 5 -> Tests: 4/4 passed, Duration: 13ms, RESULT PASS 1 test(s). Pre-fix this exact command saw Pattern A then 720 s timeout.
• Unit + integration tests for the new task topology (tracked as follow-up; user-requested).

Notes on ClearBuffer / GetInWaiting semantics

Both used to drain / report on the local rx broadcast receiver. Post-split that receiver lives in the READER task; the inbound handler cannot reach it from its own task. ClearBuffer is now a logged no-op; GetInWaiting returns 0. The pre-split use case (host discards pre-RPC boot banner before sending a command) is largely served by the READER's lag-handling now. If we need true drain semantics back, the inbound task should send a control-message into the READER's loop via an additional channel.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Refactor
  • Improved the WebSocket serial-monitor handler's internal communication architecture for better performance and reliability. Enhanced concurrent event handling and optimized message coalescing to reduce network overhead.

[coderabbitai]

Caution

Review failed

Pull request was closed or merged during review

📝 Walkthrough

Walkthrough

The handle_serial_ws function in the WebSocket serial-monitor handler is refactored from a single tokio::select! loop into three concurrent tasks (READER, WRITER, INBOUND) connected by an unbounded MPSC channel. The WRITER coalesces adjacent Data frames. ClearBuffer becomes a no-op and GetInWaiting now always returns 0.

Changes

WebSocket Serial Monitor Pipeline Refactor

Layer / File(s)	Summary
Import additions for split pipeline crates/fbuild-daemon/src/handlers/websockets.rs	Adds futures::{SinkExt, StreamExt} and tokio::sync::mpsc to support WebSocket splitting and the MPSC queue.
MPSC channel, READER task, and WRITER task with Data coalescing crates/fbuild-daemon/src/handlers/websockets.rs	Creates an unbounded MPSC channel, splits the WebSocket into sink/stream, spawns the READER task (serial broadcast → queue) and the WRITER task (queue → WebSocket sink) with logic to coalesce adjacent SerialServerMessage::Data payloads before sending.
INBOUND task, updated ClearBuffer/GetInWaiting, and session coordination crates/fbuild-daemon/src/handlers/websockets.rs	Adds the INBOUND task routing client messages to the serial manager and replies through the shared MPSC queue; ClearBuffer is now a no-op, GetInWaiting returns 0, and a final tokio::select! terminates the session when any task exits.

Sequence Diagram(s)

sequenceDiagram
  participant Client as WebSocket Client
  participant INBOUND as INBOUND Task
  participant SerialMgr as Serial Manager
  participant MPSC as MPSC Queue
  participant WRITER as WRITER Task
  participant READER as READER Task
  participant Broadcast as Serial Broadcast

  rect rgba(30, 100, 200, 0.5)
    note over READER,Broadcast: Outbound path
    Broadcast->>READER: recv() serial event
    READER->>MPSC: send(SerialServerMessage)
    MPSC->>WRITER: recv() message
    WRITER->>WRITER: coalesce adjacent Data payloads
    WRITER->>Client: send(WS frame)
  end

  rect rgba(200, 80, 30, 0.5)
    note over Client,MPSC: Inbound path
    Client->>INBOUND: WS message (write/detach/clearbuffer/getinwaiting)
    INBOUND->>SerialMgr: write bytes / detach / query
    SerialMgr-->>INBOUND: ack / error / result
    INBOUND->>MPSC: send(ack/reply SerialServerMessage)
    MPSC->>WRITER: recv() reply
    WRITER->>Client: send(ack WS frame)
  end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

• perf(serial): per-line WebSocket messages drop lines under sustained TX bursts #749: The PR directly addresses the performance degradation described in this issue by introducing the coalescing WRITER task that reduces WebSocket frame count under sustained TX bursts, and explicitly references post-perf(serial): per-line WebSocket messages drop lines under sustained TX bursts #749 handling for ClearBuffer and GetInWaiting semantic changes.

Possibly related PRs

• FastLED/fbuild#60: Directly modifies handle_serial_ws and serial-session lifecycle behavior (teardown, attach_reader state), making it code-level related to this restructuring of the same function.

Poem

🐇 Hoppity-hop, three tasks now spin,
A READER, a WRITER, INBOUND chimes in!
The frames get coalesced, less traffic to send,
ClearBuffer naps at the tunnel's far end.
No lag shall defeat my serial stream —
Async and concurrent, the rabbit's dream! 🌟

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title clearly and specifically summarizes the main architectural change: splitting the WebSocket handler into concurrent reader/writer/inbound tasks to resolve the blocking issue. It is concise, descriptive, and directly reflects the primary purpose of the refactoring.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/749-batched-serial-ws

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}