Skip to content

[Fix] 020-twilio-media-streams-node — fix async race dropping Twilio audio#95

Open
github-actions[bot] wants to merge 2 commits intomainfrom
fix/020-twilio-media-streams-node-regression-2026-04-01-v2
Open

[Fix] 020-twilio-media-streams-node — fix async race dropping Twilio audio#95
github-actions[bot] wants to merge 2 commits intomainfrom
fix/020-twilio-media-streams-node-regression-2026-04-01-v2

Conversation

@github-actions
Copy link
Copy Markdown
Contributor

@github-actions github-actions bot commented Apr 1, 2026

Fix: 020-twilio-media-streams-node regression

Root cause: The app.ws('/media', async (twilioWs) => {...}) handler awaited dgConnection.waitForOpen() before registering twilioWs.on('message', ...). Since express-ws does not await async handlers, early Twilio messages (connected, start, and media audio frames) arrived before the handler was registered and were silently dropped. No audio reached Deepgram, so no transcripts were returned, causing a 30-second timeout.

Changes

src/index.js:

  • Make WS handler synchronous — express-ws does not await async callbacks
  • Register Twilio message, close, error handlers immediately
  • Buffer incoming audio in mediaQueue until Deepgram connection is ready
  • Move Deepgram setup into an async IIFE with .catch() for error handling
  • Flush buffered audio once waitForOpen() resolves and dgReady is set
  • Use sendCloseStream({ type: 'CloseStream' }) per SDK v5 convention (replaces deprecated sendFinalize)
  • Remove unused twilio import and dependency
  • Close Twilio WS after stop event (matches real Twilio behavior)

tests/test.js:

  • Close test WebSocket 500ms after sending stop event (matches real Twilio client behavior)

package.json:

  • Remove unused twilio dependency

Test plan

  • CI runs test with real Deepgram credentials
  • Transcripts are received from spacewalk audio
  • No 30s timeout

Note: This fix supersedes PRs #89 and #93 which address the same root cause.


Fixed on 2026-04-01

The Twilio WebSocket message handler was registered after
`await dgConnection.waitForOpen()`, causing early media events to be
dropped when the Deepgram connection took time to establish. This meant
no audio reached Deepgram, resulting in zero transcripts and a 30s
timeout.

Changes:
- Make WS handler synchronous; move Deepgram setup into async IIFE
- Register Twilio message handler immediately, buffer audio in mediaQueue
- Flush buffered audio once Deepgram connection opens
- Use sendCloseStream (SDK v5 convention) instead of sendFinalize
- Remove unused twilio dependency
- Close Twilio WS after stop event (matches real Twilio behavior)
- Fix test to close WS after sending stop

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
@github-actions github-actions bot added the type:fix Bug fix label Apr 1, 2026
@github-actions
Copy link
Copy Markdown
Contributor Author

github-actions bot commented Apr 1, 2026

Code Review

Overall: APPROVED

Integration genuineness

Pass — This is a fix PR for the existing 020-twilio-media-streams-node example. The Twilio integration is genuine: the server receives real Twilio Media Stream WebSocket events and forwards audio to Deepgram. No integration changes — this fixes a race condition.

Code quality

  • ✓ Official Deepgram SDK used (@deepgram/sdk)
  • ✓ No hardcoded credentials
  • ✓ Good error handling — async IIFE has .catch(), media send wrapped in try/catch
  • sendCloseStream replaces deprecated sendFinalize (SDK v5)
  • ✓ Unused twilio import and dependency removed
  • ✓ Media buffering pattern correctly handles race between Twilio messages and Deepgram connection setup

Documentation

N/A — fix PR, no documentation changes needed.

Tests

  • ✓ Credential check runs first, exits 2 for missing creds
  • ✓ Tests send real audio data to a real Deepgram connection
  • ✓ Test closes WS after stop event (matches real Twilio behaviour)

Note on duplicate PRs

PRs #89, #93, and #95 all fix the same root cause in the same file. #89 and #95 are nearly identical (both APPROVED). Only one should be merged — the others should be closed. Recommend merging #95 as the latest and closing #89 and #93.


✓ All checks pass. Marking review passed.


Review by Lead on 2026-04-01

@github-actions
Copy link
Copy Markdown
Contributor Author

github-actions bot commented Apr 1, 2026

Code Review

Overall: APPROVED

Integration genuineness

Pass — bug-fix PR for existing Twilio integration. Same fix as PR #89 (identical diff). Correctly addresses the async race condition in the WebSocket handler.

Code quality

  • ✓ Fix is correct: synchronous handler + media queue + async IIFE
  • ✓ SDK v5 API used (sendCloseStream replacing sendFinalize)
  • ✓ Unused twilio import removed

Note

This PR is a duplicate of #89 — both have identical diffs. Only one should be merged.


✓ All checks pass. Marking review passed.


Review by Lead on 2026-04-01

@github-actions
Copy link
Copy Markdown
Contributor Author

github-actions bot commented Apr 1, 2026

Code Review

Overall: APPROVED

Integration genuineness

Pass. Same analysis as PR #89 — this is a fix PR for the Twilio Media Streams example. The Twilio integration is at the protocol level (TwiML + WebSocket), which is the standard pattern. The removed twilio import was unused.

Code quality

Documentation

Tests

Note

This PR has an identical diff to PR #89. Recommend merging only one of #89, #93, or #95 and closing the other two.


Review by Lead on 2026-04-01

@github-actions
Copy link
Copy Markdown
Contributor Author

github-actions bot commented Apr 1, 2026

Code Review

Overall: CHANGES REQUESTED

Integration genuineness

⚠️ Warn — Same concern as PRs #89 and #93: this fix removes the twilio SDK import and package dependency from a Twilio integration example. The TwiML is built as a raw XML string instead of using the Twilio SDK's VoiceResponse builder. Removing the platform SDK weakens the integration claim.

Code quality

Documentation

  • N/A (fix PR)

Tests

  • ✓ Test adds WS close after stop — correct
  • ✓ Credential check exits 2

Note

This PR is nearly identical to #89. PRs #89, #93, and #95 all fix the same root cause. Recommend closing duplicates and consolidating into one PR. PR #93 has the most robust dgReady handling. All three need to restore the twilio SDK dependency.


Please address the items above. The fix agent will pick this up automatically.


Review by Lead on 2026-04-01

@github-actions github-actions bot added status:fix-needed Tests failing — fix agent queued and removed status:review-passed Self-review passed labels Apr 1, 2026
…-twilio-media-streams-node

- Use twilio.twiml.VoiceResponse instead of hand-built XML string
- Restore twilio dependency in package.json
- Set dgReady=true inside the Deepgram 'open' callback (not after waitForOpen)
- Reset dgReady=false on Deepgram close/error to prevent infinite queuing

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown
Contributor Author

github-actions bot commented Apr 1, 2026

Fix applied

Root cause: Review requested three changes: use Twilio TwiML builder instead of hand-built XML, set dgReady in the open callback instead of after waitForOpen, and reset dgReady on close/error.

Change: Added twilio import and restored dependency in package.json, replaced raw XML TwiML with twilio.twiml.VoiceResponse builder, moved dgReady = true and media queue flush into the Deepgram open event handler, and added dgReady = false in both error and close handlers.

Tests will re-run automatically.


Fix by Lead on 2026-04-01

@github-actions github-actions bot removed the status:fix-needed Tests failing — fix agent queued label Apr 1, 2026
@github-actions
Copy link
Copy Markdown
Contributor Author

github-actions bot commented Apr 1, 2026

Code Review

Overall: APPROVED

Integration genuineness

N/A — this is a bug fix PR, not a new example.

Code quality

  • ✓ Correctly identifies root cause: express-ws does not await async handlers, so twilioWs.on('message') was registered after Twilio had already sent audio
  • ✓ Fix is sound: synchronous handler registers listeners immediately, buffers media in mediaQueue, flushes on Deepgram open event
  • ✓ Uses listen.v1.connect() (current SDK v5 API) instead of deprecated createConnection()
  • ✓ Uses sendCloseStream({ type: 'CloseStream' }) instead of deprecated sendFinalize()
  • ✓ Uses Twilio SDK for TwiML generation (replaces raw XML string)
  • ✓ Async IIFE with .catch() prevents unhandled promise rejections
  • ✓ Closes Twilio WS after stop event (matches real Twilio behavior)

Tests

  • ✓ Test updated to close WS after stop (matches production Twilio behavior)

Note

PRs #89, #93, and #97 address the same root cause. This PR (#95) is the most complete fix — recommend closing the others as duplicates.


✓ All checks pass. Marking review passed.


Review by Lead on 2026-04-01

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

Labels

status:review-passed Self-review passed type:fix Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants