Skip to content

fix(waves): close the TTS socket on every path out of a stream - #112

Closed
CaptainAni187 wants to merge 1 commit into
smallest-inc:mainfrom
CaptainAni187:fix_waves_stream_tts_socket_lifecycle
Closed

CaptainAni187 wants to merge 1 commit into
smallest-inc:mainfrom
CaptainAni187:fix_waves_stream_tts_socket_lifecycle

Conversation

@CaptainAni187

@CaptainAni187 CaptainAni187 commented Sep 12, 2026

Copy link
Copy Markdown

synthesize and synthesize_streaming call ws.close() on the line after the consume loop, so the socket is closed only when that loop runs to completion. Two ordinary paths skip it:

while True:
    if not self.error_queue.empty():
        raise self.error_queue.get()   # jumps straight past the close below
    ...
ws.close()

An error mid-stream raises out of the generator and the close never runs. A caller who stops reading early, which is just break in a for loop over the chunks, never runs it either, because abandoning a generator throws GeneratorExit at the yield. Both leak the socket and the run_forever thread behind it, and in a long-lived process those accumulate.

start_streaming_session has no ws.close() on any path, and the class exposes no public way to reach the socket, so a caller of that method has no supported way to release it at all. That method is the one meant for manual send_text_chunk / flush_buffer driving, which is exactly the case where the session outlives a single call.

A connect that times out also leaves its half-open WebSocketApp and thread behind before raising.

The change

All three generators close in a finally, which covers normal completion, the raise, and the abandoned-generator case in one place. close() becomes public and idempotent, so the manual-session caller has something to call, and _connect uses it both to replace an existing socket and to clean up after a timeout.

close() clears self.ws before closing so a second call is a no-op rather than a double close on a socket that may already be gone.

Tests

tests/custom/test_waves_stream_tts_lifecycle.py, six cases, no network: WebSocketApp is replaced with a fake that records sends and closes and drives the callbacks itself.

Five of the six fail on main, one for each leak described above. The sixth is the normal-completion path, which was never broken; it is there so the finally cannot regress it into a double close, and it asserts exactly one close.

The tests assert the socket was closed rather than that self.ws is None, so they pin the behaviour and not this particular implementation of it.

src/smallestai/waves/stream_tts.py is .fernignored, so a regen keeps this.

One thing left alone

_on_close puts the sentinel on the queue when the socket closes without is_complete, so a clean server-side close part way through a stream reads to the consumer as a normal end rather than a truncation. That is a behaviour change rather than a leak, so I left it out of this one. Happy to open it separately if you want it.

synthesize and synthesize_streaming closed the socket only after the consume
loop ran to completion, so an error raised past the close and a consumer that
stopped reading early never reached it. start_streaming_session never closed at
all and the class exposed no way to do it by hand. A connect that timed out left
its half-open socket and run_forever thread behind too.

Close in a finally on all three generators, add a public close(), and close the
half-open socket before raising the connect timeout.
@crim-app

crim-app Bot commented Sep 12, 2026

Copy link
Copy Markdown

crim doesn't review pull requests automatically here.

Comment crim review on this pull request whenever you want a review.

@CaptainAni187

Copy link
Copy Markdown
Author

crim review

@crim-app

crim-app Bot commented Sep 12, 2026

Copy link
Copy Markdown

crim is reviewing this pull request. Findings will be posted shortly.

@crim-app

crim-app Bot commented Sep 12, 2026

Copy link
Copy Markdown

Good to merge

A PR about not leaking sockets that also doesn't leak untested code paths — the fake WebSocketApp even drives its own callbacks. Ship it before the connection times out.

@crim-app crim-app Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM

Assessment

A focused, correct fix that guarantees the TTS WebSocket is closed on every exit path, backed by thorough tests.

What this PR does

Adds a public, idempotent close() method and wraps each streaming generator's consume loop in try/finally: self.close(), so the socket is released when the stream completes, raises, or is abandoned early. _connect() now routes cleanup through close(), and a failed connect closes the half-open socket before raising. New lifecycle tests exercise all paths with a fake WebSocketApp.

Findings

No issues found. The close() swap-and-none pattern is safe to call repeatedly, the finally blocks cover completion/error/early-exit, and the documented manual-session contract for start_streaming_session is consistent with its implementation.

@abhishekmishragithub

Copy link
Copy Markdown
Collaborator

Superseded by #120 (re-homed onto an upstream branch so CI could run; your commit is included with authorship preserved, and the test type-check gaps for #112/#118 were fixed there). Shipping in 5.12.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants