Fix use-after-free in async TTS progress callbacks - #3781
Merged
csukuangfj merged 2 commits intoJul 23, 2026
Merged
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
dosubot
Bot
added
the
size:L
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAsync offline TTS progress handling now coordinates generation and callback-queue completion through shared settlement state, bounded TSFN queues, cancellation/error propagation, and done sentinels. A Node.js stress test covers sequential, cancellation, throwing, concurrent, RSS, and buffer-marshalling scenarios. ChangesAsync TTS callback lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant NodeTest
participant OfflineTts
participant TtsGenerateWorker
participant TSFN
NodeTest->>OfflineTts: generateAsync with onProgress
OfflineTts->>TtsGenerateWorker: start generation
TtsGenerateWorker->>TSFN: queue progress chunks
TtsGenerateWorker->>TSFN: queue done sentinel
TSFN->>OfflineTts: deliver callbacks and completion
OfflineTts-->>NodeTest: resolve or reject promise
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
nodejs-addon-examples/test_tts_async_callback_stress.js (1)
50-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that progress callbacks actually fire in
sequential()andcopyBuffer().Both functions track a chunk counter but never assert it's non-zero, unlike
cancellation()/throwing()/concurrent()which strictly check callback counts. Since this PR's bug class is exactly "callbacks silently lost/UAF'd," a regression that drops callbacks entirely (0 chunks) would pass these two paths undetected.
nodejs-addon-examples/test_tts_async_callback_stress.js#L50-L78: after the loop (or per-iteration), assertchunks > 0(and/ortotalChunks >= iterations) before logging.nodejs-addon-examples/test_tts_async_callback_stress.js#L198-L215: assertchunks > 0alongside the existingr.samples.lengthcheck.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nodejs-addon-examples/test_tts_async_callback_stress.js` around lines 50 - 78, Strengthen callback coverage in sequential() by asserting each generation produces at least one progress callback, or equivalently that totalChunks meets the expected iteration count, before logging. Also update copyBuffer() at nodejs-addon-examples/test_tts_async_callback_stress.js lines 198-215 to assert chunks > 0 alongside the existing r.samples.length check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@harmony-os/SherpaOnnxHar/sherpa_onnx/src/main/cpp/non-streaming-tts.cc`:
- Around line 1019-1032: Update both TtsGenerateWorker::OnOK
(non-streaming-tts.cc lines 1019-1032) and TtsGenerateWithConfigWorker::OnOK
(non-streaming-tts.cc lines 1237-1250) so the sentinel-failure finalization also
runs when tsfn_closing_ is true; preserve the existing error assignment and
force callbacks_drained before SettleOrFail so the promise settles without a
done sentinel.
- Around line 793-824: Update SettleIfReady to validate state->audio immediately
after generation and before either result-building branch. If it is null, reject
the promise and return without dereferencing or transferring the audio pointer;
preserve the existing external- and internal-buffer handling for valid audio.
---
Nitpick comments:
In `@nodejs-addon-examples/test_tts_async_callback_stress.js`:
- Around line 50-78: Strengthen callback coverage in sequential() by asserting
each generation produces at least one progress callback, or equivalently that
totalChunks meets the expected iteration count, before logging. Also update
copyBuffer() at nodejs-addon-examples/test_tts_async_callback_stress.js lines
198-215 to assert chunks > 0 alongside the existing r.samples.length check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 632df1bc-96b2-4acf-af42-0468cc3a3907
📒 Files selected for processing (3)
.github/scripts/test-nodejs-addon-npm.shharmony-os/SherpaOnnxHar/sherpa_onnx/src/main/cpp/non-streaming-tts.ccnodejs-addon-examples/test_tts_async_callback_stress.js
There was a problem hiding this comment.
Pull request overview
This PR fixes a deterministic use-after-free crash in the Node.js addon’s OfflineTts.generateAsync() progress-callback path by reworking TSFN queue ownership and ensuring queued callbacks are drained before the generation promise is settled (shared with HarmonyOS via symlink).
Changes:
- Refactors async TTS progress callback delivery to use single-owner heap chunks (RAII) and a bounded TSFN queue with backpressure, plus a FIFO “done” sentinel to guarantee drain-before-settle.
- Adjusts cancellation/error behavior so callback cancellation/throws stop further callbacks and influence promise settlement deterministically.
- Adds a new Node.js stress test for repeated, concurrent, cancelled, and throwing progress callbacks, and runs it in the existing CI script.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| harmony-os/SherpaOnnxHar/sherpa_onnx/src/main/cpp/non-streaming-tts.cc | Reworks async generation callback plumbing to fix lifetime ordering and settle only after callback drain. |
| nodejs-addon-examples/test_tts_async_callback_stress.js | Adds a stress/regression test for async progress callbacks, cancellation, throwing callbacks, concurrency, and buffer-copy path. |
| .github/scripts/test-nodejs-addon-npm.sh | Runs the new stress test in the Node.js addon CI script. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return; | ||
| } | ||
|
|
||
| Napi::Object ans = Napi::Object::New(env); |
Comment on lines
+913
to
+916
| if (env.IsExceptionPending()) { | ||
| Napi::Error e = env.GetAndClearPendingException(); | ||
| error_message = e.Message(); | ||
| cancel_requested = true; |
| @@ -0,0 +1,230 @@ | |||
| // Copyright (c) 2026 Kevin Castillo | |||
csukuangfj
approved these changes
Jul 23, 2026
csukuangfj
left a comment
csukuangfj
left a comment
Collaborator
There was a problem hiding this comment.
Thank you for your contribution!
This was referenced Aug 12, 2026
This was referenced Sep 15, 2026
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3780
Problem
OfflineTts.generateAsync()with anonProgresscallback can deterministically abort the process during multi-chunk generation (full stack and minimal reproduction in #3780). Root cause: the TSFN queue holds raw pointers toTtsCallbackDataobjects owned by the AsyncWorker'sdata_list_, and nothing orders queue drain before the worker destructor frees them.Changes
napi_closingwithout accessing the TSFN again.onProgressthrows.generationConfigasync workers.The file is shared with the HarmonyOS binding via symlink, so both platforms get the fix.
Behavior change
A throwing
onProgresspreviously invoked undefined behavior (the exception unwound through the N-API boundary). It now cancels the generation (best-effort) and the promise rejects with an error containing the thrown callback message. Exceptions never cross the N-API boundary, in both node-addon-api exception modes.Testing
Added
test_tts_async_callback_stress.js, covering:onProgress(one callback total; no callbacks after cancellation).uncaughtException).enableExternalBuffer: false.The test is included in the existing Node.js addon CI script using the Piper English model already downloaded by that workflow.
Tested locally with:
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
Tests