Skip to content

fix(tui_gateway): preserve websocket batch order - #69684

Merged
OutThisLife merged 3 commits into
NousResearch:mainfrom
helix4u:agent/ws-stream-ordering
Jul 23, 2026
Merged

OutThisLife merged 3 commits into
NousResearch:mainfrom
helix4u:agent/ws-stream-ordering

Conversation

@helix4u

@helix4u helix4u commented Jul 22, 2026

Copy link
Copy Markdown
Collaborator

What does this PR do?

Serializes WebSocket writes per WSTransport across whole coalesced batches so streamed Desktop/dashboard frames cannot arrive out of order when the event loop recovers from a long stall.

The token buffer's threading.Lock currently preserves buffer-drain and task-creation order, but it does not serialize the asynchronous socket writes. Multiple _safe_send_many() tasks can therefore be in flight together, and each task yields at await send_text(). A later batch can enter the socket between two frames from an earlier batch. A deterministic reproduction produced A1, B1, B2, A2 from logically ordered batches [A1, A2] and [B1, B2].

This PR carries forward the per-socket serialization work from #48446 with the original contributor's authorship preserved, then adapts it to the token-batching implementation that landed afterward. It does not reopen the WebSocket-disconnect diagnosis resolved by #55545; this fixes the separate frame-ordering invariant.

Related Issue

Related to #48446 and #55545.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • tui_gateway/ws.py: add a per-transport async writer lock and hold it for each complete _safe_send_many() batch.
  • tui_gateway/ws.py: route single-frame sends through the same serialized writer path.
  • tui_gateway/ws.py: send tokens drained by write_async() and its current control/RPC frame in one lock acquisition so another batch cannot overtake the current frame.
  • tests/test_tui_gateway_ws.py: retain the original concurrent-send regression and add deterministic cross-batch and write_async() ordering coverage.

How to Test

  1. Run tests/test_tui_gateway_ws.py through the project test runner.
  2. Confirm test_ws_transport_preserves_cross_batch_order blocks batch B until every frame in batch A has been sent.
  3. Confirm test_ws_write_async_keeps_drained_tokens_with_current_frame keeps the current frame immediately behind the pending tokens it drains.

Focused local validation on Windows 11:

  • python -m py_compile tui_gateway/ws.py tests/test_tui_gateway_ws.py passed.
  • Direct isolated execution of the three serialization regressions passed.
  • git diff --check passed.
  • Full pytest coverage is left to GitHub CI.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: Windows 11

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — N/A, no user-facing contract changed
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A, no config changed
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A, no workflow changed
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — the fix uses platform-neutral asyncio.Lock
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A, no model tool changed

Screenshots / Logs

Before the fix, the deterministic batch reproduction entered the socket as A1, B1, B2, A2. With the serialized batch boundary, it enters as A1, A2, B1, B2.

@alt-glitch alt-glitch added type/bug Something isn't working comp/tui Terminal UI (ui-tui/ + tui_gateway/) P2 Medium — degraded but workaround exists sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Jul 22, 2026
@helix4u
helix4u marked this pull request as ready for review July 23, 2026 00:07
The batch-serialization fix routes every send through _safe_send_many;
_safe_send became a dead single-line wrapper with no callers. Remove it.

@OutThisLife OutThisLife left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve. Confirmed the ordering bug on current main: multiple _safe_send_many batches are scheduled as independent tasks with nothing serializing them across the await send_text() yield, so a loop stall interleaves them (A1,B1,B2,A2). The per-transport asyncio.Lock held across the whole batch — plus folding pending tokens + the control frame into one locked batch in write_async — closes both slip-in paths, and the ordering-invariant tests cover them. supplefrog's original #48446 work is preserved. Pushed one tiny follow-up removing the now-dead _safe_send wrapper. Enabling auto-merge.

@OutThisLife
OutThisLife enabled auto-merge (squash) July 23, 2026 04:56
@OutThisLife
OutThisLife disabled auto-merge July 23, 2026 04:58
@OutThisLife
OutThisLife enabled auto-merge (squash) July 23, 2026 04:59
@OutThisLife
OutThisLife merged commit 8e01309 into NousResearch:main Jul 23, 2026
34 checks passed
randlee pushed a commit to randlee/hermes-agent that referenced this pull request Aug 11, 2026
* fix: serialize TUI gateway websocket sends

* fix(tui_gateway): preserve websocket batch order

* refactor(tui_gateway): drop unused _safe_send wrapper

The batch-serialization fix routes every send through _safe_send_many;
_safe_send became a dead single-line wrapper with no callers. Remove it.

---------

Co-authored-by: supplefrog <78985073+supplefrog@users.noreply.github.com>
Co-authored-by: Brooklyn Nicholson <brooklyn.bb.nicholson@gmail.com>
prmartinow pushed a commit to prmartinow/hermes-agent that referenced this pull request Aug 26, 2026
* fix: serialize TUI gateway websocket sends

* fix(tui_gateway): preserve websocket batch order

* refactor(tui_gateway): drop unused _safe_send wrapper

The batch-serialization fix routes every send through _safe_send_many;
_safe_send became a dead single-line wrapper with no callers. Remove it.

---------

Co-authored-by: supplefrog <78985073+supplefrog@users.noreply.github.com>
Co-authored-by: Brooklyn Nicholson <brooklyn.bb.nicholson@gmail.com>
melon-xf added a commit to melon-xf/hermes-agent that referenced this pull request Sep 3, 2026
* fix: serialize TUI gateway websocket sends

* fix(tui_gateway): preserve websocket batch order

* refactor(tui_gateway): drop unused _safe_send wrapper

The batch-serialization fix routes every send through _safe_send_many;
_safe_send became a dead single-line wrapper with no callers. Remove it.

---------

Co-authored-by: supplefrog <78985073+supplefrog@users.noreply.github.com>
Co-authored-by: Brooklyn Nicholson <brooklyn.bb.nicholson@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/tui Terminal UI (ui-tui/ + tui_gateway/) P2 Medium — degraded but workaround exists sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants