-
Couldn't load subscription status.
- Fork 166
Fix race condition causing "Writer is not bound to a WritableStream" error in Silero VAD #786
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: dbb6c0e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mention that the Close Thread calls outputWriter.releaseLock() .... closed = true but we don't multi thread? It's asynchronous. Are you sure this is the root cause?
Are you able to reliably reproduce this issue? If so can you list the steps?
Yes, I'm pretty sure. This is "async interleaving", there are await inside the both yield control back to the event loop. Here's how the race condition occurs:
|
plugins/silero/src/vad.ts
Outdated
| function isWriterReleaseError(e: unknown): boolean { | ||
| if (e instanceof TypeError) { | ||
| // Check for ERR_INVALID_STATE error code (most reliable) | ||
| if ('code' in e && e.code === 'ERR_INVALID_STATE') { | ||
| return true; | ||
| } | ||
|
|
||
| // Fallback to message checking for compatibility | ||
| const message = e.message; | ||
| return ( | ||
| message.includes('Writer is not bound to a WritableStream') || | ||
| message.includes('WritableStream is closed') | ||
| ); | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this hacky? Should we have another mechanism of synchronization?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have similar approach in defered_stream.ts
agents-js/agents/src/stream/deferred_stream.ts
Lines 17 to 28 in 7fc7808
| export function isStreamReaderReleaseError(e: unknown) { | |
| const allowedMessages = [ | |
| 'Invalid state: Releasing reader', | |
| 'Invalid state: The reader is not attached to a stream', | |
| ]; | |
| if (e instanceof TypeError) { | |
| return allowedMessages.some((message) => e.message.includes(message)); | |
| } | |
| return false; | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error we're catching (ERR_INVALID_STATE: Writer is not bound) is essentially Node.js telling us the stream is shutting down. It's not really an exceptional condition, it's an expected part of the shutdown sequence that we need to handle gracefully. The safeWriteEvent() method makes this explicit: it catches expected shutdown errors and signals the loop to exit cleanly, while still throwing any truly unexpected errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this happen in llm/stt/tts as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@toubatbrian I think this is what you mean. We have a to await the model to actually run during which the outputWriter might be closed.
(model run)
agents-js/plugins/silero/src/vad.ts
Lines 221 to 223 in 7fc7808
| const p = await this.#model | |
| .run(inferenceData) | |
| .then((data) => this.#expFilter.apply(1, data)); |
(output writer definition)
Line 91 in 7fc7808
| protected outputWriter: WritableStreamDefaultWriter<VADEvent>; |
Is there a simpler solution where the base vad class has a method that wraps this.outputWriter.write with a if (!this.closed)? Then every plugin just calls this method and doesn't have to worry about the internals of outputWriter?
@Shubhrakanti Great question! Let's definitely explore that idea later. For now let's focus on fixing the VAD issue within the scope of this PR to unblock our customer first. We can revisit and discuss a cleaner solution afterward. |
All we need to do is add one method to the base vad class writeOutput(event: VADEvent) {
if (!this.closed) {
this.outputWriter.write(event)
} else {
// you can either throw an error or have a boolean which returns if the write was successful or not. Or just do nothing and the next iteration of `this.#task` in silerio.vad the main loop will break since `this.close == true`
....
}and then you replace It's fine if this goes out Mondaynext week. I'd rather have it done correctly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks a lot cleaner. Just a few questions that would be good to get the answer for. Mainly behind why we still need isWriterReleaseError. Can't we just sync the writer state with this.closed? cc: @theomonnom - let me know if I'm focussing on the wrong details here.
| return true; | ||
| } catch (e) { | ||
| // Ignore writer release errors during stream closure (e.g., on participant disconnect) | ||
| if (isWriterReleaseError(e)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this.closed is false then the writer should not be closed right? We should never see this error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can drop this, but I’m keeping it for now as an extra safety measure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's drop it for now. If we see this error again that means the writer state is out of sync with this.closed. We want to know that so we can fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please look at the two comments before merging.
| if ( | ||
| !this.sendVADEvent({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!this.sendVADEvent) {
continue
}Feels redundant? We can just do this.sendVADEvent? We don't need a return type?
Problem
The Silero VAD plugin was intermittently throwing unhandled
TypeError: Invalid state: Writer is not bound to a WritableStreamerrors when plugin close.Root Cause
A race condition exists between the VAD inference loop and the stream shutdown process:
while (!this.closed)before processing framesclose()method:outputWriter.releaseLock()to unbind the writer from the streamclosed = true!this.closedcheck but then attempt to write to an already-released writerRace Condition Timeline:
Solution
Implemented graceful error handling following the established pattern in
deferred_stream.ts.