fix: serialize TUI gateway websocket sends - #48446
Conversation
|
Related: #42938 (Windows ProactorEventLoop |
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
✅ Looks Good
- Well-scoped fix: Only 2 files touched (ws.py + test), addressing exactly the issue described in #48445
- Clear root cause: The comment explains the problem — concurrent
send_text()calls from multiple worker paths during high-output turns can overlap and cause issues - Correct solution:
asyncio.Lock()properly serializes writes; check ofself._closedinside the lock prevents sending on a closed socket - Good test coverage: New test (
test_ws_transport_serializes_concurrent_sends) verifies:- Two concurrent writes don't overlap (
max_active_sends == 1) - Both writes complete successfully
- Transport remains open after concurrent writes
- Two concurrent writes don't overlap (
- No debug artifacts or secrets
- Follows existing patterns in the file (async lock pattern consistent with other async code in the codebase)
Minor Note
- The test uses
asyncio.sleep(0.05)to simulate work — this is appropriate for a concurrency test.
Reviewed by Hermes Agent
|
Closing in favor of #55545, which merged the GIL-pressure WS-disconnect fix to Your PR correctly identified the disconnect symptom, but the serialize-sends approach targets the "overlapping The merged fix addresses that directly — loopback ws-ping window widened to ride out stalls, per-token frame coalescing to cut loop wakeups, and frontend-polled RPCs routed off the read loop. Thanks for the well-mapped report on the original issue (#48445) — the file/line citations made tracing this fast. |
Summary
send_text()on the same socketFixes #48445
Test Plan
.venv/Scripts/python.exe -m pytest tests/test_tui_gateway_ws.py -q -o addopts=— 4 passed.venv/Scripts/python.exe -m pytest tests/test_tui_gateway_server.py tests/test_tui_gateway_ws.py -q -o addopts=— 281 passed, 1 warningNotes
tests/on native Windows currently has unrelated baseline failures tracked in Windows-native pytest suite has baseline failures in ACP, path, CRLF, and file-handle tests #48986. This PR was verified with the affected TUI gateway test slice.