-
Notifications
You must be signed in to change notification settings - Fork 214
fix: handle VAD stream closed error during handover and upgrade sharp #997
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
Conversation
🦋 Changeset detectedLatest commit: e273f4b The changes in this PR will be included in the next version bump. This PR includes changesets to release 18 packages
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 |
📝 WalkthroughWalkthroughFix race-condition where StreamAdapter could call Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🔇 Additional comments (2)
✏️ Tip: You can disable this entire section by setting Comment |
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.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/test/src/stt.ts (1)
61-67: Pre-existing issue:endInput()called inside the loop.The
stream.endInput()call is inside theforloop, meaning it's called after every frame instead of once after all frames are pushed. This appears to be a pre-existing bug unrelated to this PR's changes.const input = async () => { for (const frame of frames) { stream.pushFrame(frame); await new Promise((resolve) => setTimeout(resolve, 5)); - stream.endInput(); } + stream.endInput(); };
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.changeset/wet-carpets-clean.mdagents/src/stt/stream_adapter.tsagents/src/utils.tsplugins/test/src/stt.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/agent-core.mdc)
Add SPDX-FileCopyrightText and SPDX-License-Identifier headers to all newly added files with '// SPDX-FileCopyrightText: 2025 LiveKit, Inc.' and '// SPDX-License-Identifier: Apache-2.0'
Files:
agents/src/utils.tsplugins/test/src/stt.tsagents/src/stt/stream_adapter.ts
**/*.{ts,tsx}?(test|example|spec)
📄 CodeRabbit inference engine (.cursor/rules/agent-core.mdc)
When testing inference LLM, always use full model names from
agents/src/inference/models.ts(e.g., 'openai/gpt-4o-mini' instead of 'gpt-4o-mini')
Files:
agents/src/utils.tsplugins/test/src/stt.tsagents/src/stt/stream_adapter.ts
**/*.{ts,tsx}?(test|example)
📄 CodeRabbit inference engine (.cursor/rules/agent-core.mdc)
Initialize logger before using any LLM functionality with
initializeLogger({ pretty: true })from '@livekit/agents'
Files:
agents/src/utils.tsplugins/test/src/stt.tsagents/src/stt/stream_adapter.ts
🧠 Learnings (2)
📚 Learning: 2026-01-16T14:33:39.551Z
Learnt from: CR
Repo: livekit/agents-js PR: 0
File: .cursor/rules/agent-core.mdc:0-0
Timestamp: 2026-01-16T14:33:39.551Z
Learning: Applies to examples/src/test_*.ts : For plugin component debugging (STT, TTS, LLM), create test example files prefixed with `test_` under the examples directory and run with `pnpm build && node ./examples/src/test_my_plugin.ts`
Applied to files:
plugins/test/src/stt.ts
📚 Learning: 2026-01-16T14:33:39.551Z
Learnt from: CR
Repo: livekit/agents-js PR: 0
File: .cursor/rules/agent-core.mdc:0-0
Timestamp: 2026-01-16T14:33:39.551Z
Learning: Applies to **/{examples,test}/**/*.test.ts : Include both basic streaming and tool calling tests to verify full LLM functionality
Applied to files:
plugins/test/src/stt.ts
🧬 Code graph analysis (2)
agents/src/utils.ts (1)
agents/src/transcription.ts (1)
Error(302-306)
agents/src/stt/stream_adapter.ts (1)
agents/src/utils.ts (1)
isStreamClosedError(685-690)
🔇 Additional comments (4)
.changeset/wet-carpets-clean.md (1)
1-6: LGTM!The changeset correctly marks both affected packages as patches and provides a clear description of the fix.
agents/src/stt/stream_adapter.ts (1)
72-82: LGTM! Clean fix for the race condition.The try/catch guard around
endInput()correctly handles the case whereclose()is called whileforwardInputis still running. The early return for stream-closed errors is appropriate, and re-throwing other errors preserves the original error handling behavior.plugins/test/src/stt.ts (1)
143-150: LGTM! Correct ArrayBuffer conversion.The conversion properly handles the
Int16Arrayview by usingbyteOffsetandbyteLengthto extract the exact portion of the underlying buffer. This is the correct way to convert a typed array view to a standaloneArrayBuffer.agents/src/utils.ts (1)
678-690: LGTM! Well-documented utility for stream cleanup.The function correctly handles the type narrowing and checks for both error messages that are actually thrown throughout the codebase (in vad.ts, tokenizer.ts, token_stream.ts, and stt.ts). The JSDoc clearly explains the purpose and usage context.
One consideration: String-based error matching is somewhat fragile if upstream libraries change their error messages. For critical paths, consider verifying these exact strings match what the stream implementations actually throw.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Summary
StreamAdapterthat causedstt_error: "Stream is closed"during agent handoverisStreamClosedError()utility function for consistent error handling@livekit/agents-plugins-testProblem
@img/sharp-libvips-darwin-arm64(1.2.0 and 1.2.4) caused ObjC class collisions and flaky agent behaviorTest Plan
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.