Repository navigation
fix(websocket): drain httpx-ws reader thread before closing sync sessions - #164
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 31 minutes. View limit details
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Reader draining utility src/fishaudio/core/_ws_utils.py, pyproject.toml |
Adds drain_reader, which sends a close frame, shuts down the socket, and joins the background reader thread. The minimum httpx-ws version changes from 0.6.2 to 0.7.2. |
TTS cleanup integration src/fish_audio_sdk/websocket.py, src/fishaudio/resources/tts.py |
Calls drain_reader in finally blocks after synchronous TTS receive processing and before WebSocket cleanup. |
Cleanup validation tests/unit/test_ws_utils.py |
Tests close-frame behavior, socket shutdown, reader termination, failure handling, and both current and legacy TTS WebSocket paths. |
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Bug fix
Suggested reviewers: abersheeran
Merge Risk: 
9ec77
-
A receive-side TTS failure can leave its sender running during WebSocket cleanup, creating a bounded stability risk that should be followed up.
Pre-merge checks: 
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: draining the httpx-ws reader thread before closing synchronous WebSocket sessions. Docstring Coverage Docstring coverage is inconclusive: files from earlier reviews still need to be rechecked within the check's analysis limits. Not blocking. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Finishing Touches
-
Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
Codecov Report✅ All modified and coverable lines are covered by tests.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
090c91b to
2e0fe43
Compare
leng-yue
left a comment
leng-yue
left a comment
There was a problem hiding this comment.
Reviewed. Verified the drain against httpx-ws 0.6.2/0.7.2/0.9.0 with a local WebSocket server (plain + TLS); raised the floor to httpx-ws>=0.7.2 because older versions spin for the full 5 s join. Unit tests green on 3.9–3.13; integration tests (52) pass against the live API on the same head via an in-repo branch.
…anism notes; real-socket test
Before 0.7.2 the httpx-ws reader thread does not exit on EOF after shutdown(SHUT_RDWR); it spins until close() sets _should_close, so drain_reader's join waited the full 5 s on a busy core whenever the server did not answer the close frame. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
9ec7700 to
c8129b8
Compare
Symptom
Intermittent
[SSL: WRONG_VERSION_NUMBER],record layer failure, or[X509] PEM liberrors on the WebSocket session opened right after a completed sync WebSocket TTS session (client.tts.stream_websocket(...)and legacyWebSocketSession.tts(...)). Linux only; worse on loaded hosts. The async client is unaffected.Mechanism
httpx-ws's sync
WebSocketSessionreads on a background thread and closes the socket from the calling thread (close()writes the close frame, thenstream.close()).A read that has not started yet is harmless: CPython's
SSLSocket._real_closesets_sslobj = Nonebefore closing the fd, so a later read falls through tosocket.recvon fd-1and getsEBADF.The culprit is the read that is already in flight. On Linux,
close()does not interrupt a thread blocked inrecv(). OpenSSL (read_ahead off) fetches a TLS record header and body with separateread(fd)calls on the fd number cached in its BIO. When the server's close-frame reply arrives, the secondread()lands on the next connection, which has reused the same fd number, and that new session fails during its TLS handshake / 101 read. macOS wakes the blocked reader withEBADFonclose(), which is why it is unaffected there.Fix
fishaudio.core._ws_utils.drain_reader(ws), called in afinallybefore leaving thewith connect_ws(...)block in both sync paths (fishaudio/resources/tts.pyandfish_audio_sdk/websocket.py). It:CloseConnection(1000)viaws.send(...), so the server still sees a clean 1000 close (not 1006) and wsproto moves toLOCAL_CLOSING, which makes httpx-ws's laterclose()skip its own frame;shutdown(SHUT_RDWR)on the socket fromws.stream(fallback:response.extensions["network_stream"]), which wakes the in-flight read with EOF;_background_receive_task(5 s timeout; only reached when the reader is stuck, where httpx-ws's own untimed join would hang afterwards anyway).Everything is best-effort; failures are logged at debug. The sender future is still awaited/raised as before. Async paths untouched.
Validation
GCP us-central1 Linux VM (Python 3.12, httpx-ws 0.9.0), 8 parallel processes x 10 sessions each, per batch:
new0/80,legacy0/80,new0/80,legacy0/80)The remaining non-SSL failures in those batches (19/320,
WebSocket disconnected unexpectedly/WebSocketErr) are a separate server-side issue that also appears without this patch.Unit tests: helper ordering (close frame, shutdown, join), stream fallback, send failure, shutdown failure, missing socket/thread, debug logging on unexpected errors; both sync paths call the helper before
__exit__(asserted onmock_callsorder); and a realhttpx_ws.WebSocketSessionover asocket.socketpair()that checks the reader thread exits, nothreading.excepthookfires, state isLOCAL_CLOSING, and the peer receives a 1000 close frame before FIN. Passes on Python 3.9 (httpx-ws 0.7.2), 3.12 (0.8.1) and 3.13 (0.9.0).Residual risk
If httpx-ws has already closed the socket on one of its own error paths (keepalive ping timeout, or a sender
WriteErrortriggering itsclose()), the fd is already freed,shutdown()fails withEBADF, and the in-flight read can still race. That can only be fixed inside httpx-ws'sclose()(shut down / join before closing the fd). Upstream fix: frankie567/httpx-ws#150. Once that lands and we bump the dependency, this helper becomes redundant but harmless.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by CodeRabbit
Bug Fixes
Tests