fix(sherpa-onnx.rn/android): let stopTts interrupt an in-flight generation - #463
Merged
Conversation
added 2 commits
August 21, 2026 16:45
…ation init, generate, stop and release share one single-threaded executor, so stop() queued behind the generation it was meant to interrupt. The isGenerating checks inside the callback loop could never fire during that generation, making stopTts() a no-op until generation had already finished. isGenerating is cleared on the calling thread now, before queueing. It is @volatile, so the loop sees the write immediately. The AudioTrack teardown stays on the executor, because the generation loop writes to it from there and pausing it across threads is the hazard the serialization was protecting against. release() had the same defect and gets the same treatment. A/B on Pixel 6a via testTTSStopDuringPrefill(120), stop requested ~120ms in: main: generation 4957ms, 112610 samples in 1 callback -> stop ignored fixed: generation 826ms, 0 samples in 0 callbacks -> stop interrupted On main the "Stopping TTS generation" line lands 1.4s AFTER generation completes; with the fix generation aborts having emitted nothing. Worth recording: the log line alone appears in both, so presence proves nothing — the duration and sample count are what distinguish them. My first attempt at this A/B checked only for the line and would have passed either way. The app still SIGSEGVs afterwards delivering results to JS, 3.2s after the stop fires and entirely in Hermes frames. That is #436, unrelated to this change and present on main too.
…d the fallback Review found the boolean flag fix was too shallow. Two ways it still lost a cancellation, both confirmed in source. A queued generation swallowed the stop. generate() set isGenerating=true only inside its executor task, so a stop arriving after generate() was queued but before it ran cleared an already-false flag. The queued task then set it true and ran to completion ahead of the stop, which resolved as success having interrupted nothing. A boolean cannot express this: "already false" and "nothing to cancel" are the same value. Replaced with a monotonic request id. generate() claims an id on the calling thread before queueing; stop()/release() raise cancelledThrough to the newest claimed id, cancelling everything outstanding whether started or not. A generation started after the cancel keeps a higher id and is unaffected. The fallback undid the cancellation. generateWithCallback discards its return, so after an interrupt `audio` is null and control fell into the plain tts.generate() fallback, which ignores cancellation entirely. That restarted synthesis and resolved it as success — reversing the fix outright. It is also the path playAudio=false takes, which is the service default, so the common case was the broken one. Now checks the token before falling back. Also updated the prefill guard, whose comment called the check defensive on the grounds that stop() could not run mid-generation. That is no longer true — the fix is precisely that it can — so the check is load-bearing and now consults the token. And the testTTSStopDuringPrefill comment in agentic-bridge.ts still said Android could not interrupt at all; it now says what to look for, including that the "Stopping TTS generation" log line appears in both fixed and unfixed builds and only duration and sample count distinguish them. Kotlin compiles (:siteed_sherpa-onnx.rn:compileDebugKotlin BUILD SUCCESSFUL). Device validation pending.
deeeed
force-pushed
the
fix/tts-stop-interrupt-440
branch
from
August 21, 2026 09:22
2a60f83 to
acd0d4b
Compare
…lls, not only before
Round-2 review found the token was consulted before every blocking call and
after none of them, so a stop arriving during synthesis still reported success.
`tts.generate()` cannot be interrupted at all, and it is what `playAudio=false`
uses — the service default. The guard added last round sat before that call, so
the entire synthesis window was unchecked: a stop during it fell straight
through to `promise.resolve({success: true})`. The fallback path had the same
shape. Both branches converge before the result is built, so one check there
covers both.
Playback could also start after a cancel. The callback checked cancellation on
entry, but the chunk loop and the blocking `AudioTrack.write()` that follow can
span a stop, and `stop()`'s pause/flush is queued behind this generation — so it
cannot undo a `play()` that already ran. Both `play()` sites now re-check
immediately before starting: the prefill-threshold one in the chunk loop, and
the short-utterance tail.
The CAS loop was confirmed correct by the reviewer's stress probe (100 rounds,
1000 concurrent raises), so it is unchanged.
Kotlin compiles and the unit tests pass. Device validation still pending — the
A/B that distinguishes a working stop from a no-op is duration and sample count,
not the log line, which appears in both builds.
Collaborator
Author
Device validation (Pixel 6a, fresh install)
Ran Three things this shows, in order of what they prove:
Worth stating explicitly since I got this wrong once before: the Unrelated crash in the same sessionThe app died ~3s later: That is #436, not this change — the TTS work had completed at 18:02:52.188, and the signature ( IncidentalThe model URL in Compile: |
added 5 commits
August 21, 2026 23:16
…ating the flag
Four findings from review, all real.
The callback checked cancellation only on entry, so a stop landing mid-chunk kept
issuing blocking writes and returned 1, telling the engine to carry on while
stop()'s pause/flush sat queued behind the generation. It is now checked every
chunk.
The tts.generate() fallback checked the token before the call but not after. That
call blocks for the whole synthesis and cannot be interrupted, so a stop during
it still saved the file and resolved success.
Both play() sites were check-then-act. A cancel landing between isActive() and
play() started playback that stop() could not undo, its pause/flush being queued
behind us. Each site now rechecks immediately after play() and pauses and flushes
the track itself, since it is the only actor that can act before the executor
drains.
cancelOutstandingRequests snapshotted the counter and then cleared the shared
isGenerating flag. A request claiming an id after that snapshot is not covered by
the cancel, but its callback aborted on the global flag anyway. The flag is no
longer touched there and no longer consulted as a cancellation signal — the
request id already names exactly which requests are cancelled, and the generation
that owns the flag clears it in its own finally.
Device evidence. The reviewer noted my previous proof had zero callback calls, so
it never exercised these windows. This run establishes a baseline first:
uninterrupted 118784 samples, 1 callback call, playState 3
stopped at 1500ms 45056 samples, "interrupted mid-chunk by stop request",
"Error generating speech: TTS generation was cancelled"
62% truncated, through the new in-loop check rather than the entry check, which
is the path the earlier prefill-only test could not reach.
Kotlin compiles and the 16 unit tests pass.
…on atomic
Round-4 review found three, including one I thought I had already fixed.
There are two call sites for the blocking tts.generate(). I guarded one last
round and never looked for another. The one I missed is the path that actually
runs: the callback's return value is discarded, so audio is null and every
uninterrupted playAudio=true request reaches it. A stop during that call still
saved the WAV and resolved success. Both are guarded now.
Cancellation and completion were not atomic. A recheck before the file write
narrows the window but cannot close it — the write is synchronous, so a stop
landing partway through left the request cancelled and resolved at once.
Completion is now claimed through tryCompleteRequest under the same monitor
cancelOutstandingRequests takes, so exactly one of the two wins. Both completion
sites claim before writing.
stop()'s queued cleanup paused and flushed the shared AudioTrack unconditionally,
including for a generation that claimed its id after the cutoff and is therefore
not cancelled by that stop. It now compares the counter against the cutoff it
established and leaves a newer generation's track alone.
Device-verified, fresh install (lastUpdateTime 12:31:03), both directions,
because the completion claim sits in the success path too:
uninterrupted 115456 samples, "Audio saved: true", no cancel
stopped at 1500ms 45056 samples, "interrupted mid-chunk by stop request",
"TTS generation was cancelled", no file written
Kotlin compiles and the 16 unit tests pass.
…wing set
Round-5 review found two.
My stop cleanup compared requestCounter against the cutoff, which says a newer
request exists — not that it owns the track. Call stopTts() and immediately queue
a playAudio=false generate: the counter bumps while the cancelled request's track
is still playing, cleanup skips it, and nothing else ever releases it, since the
new request has no playback. The owner is now recorded when the track is built
and cleared when it is released, and stop() compares that instead.
completedRequests was only ever a monitor. Nothing read or removed its entries,
so it grew by one boxed Long per successful synthesis. Replaced with a plain lock
object, which gives the same mutual exclusion between cancel and complete.
Device-verified, fresh install (lastUpdateTime 12:43:13), both directions:
uninterrupted 105216 samples, "Audio saved: true"
stopped at 1500ms 45056 samples, "interrupted mid-chunk by stop request",
"TTS generation was cancelled", and "Paused and flushed
AudioTrack"
That last line is the point: the cancelled request owns the track, so cleanup
runs. The skip path exists for a track a newer generation owns.
Kotlin compiles and the 16 unit tests pass.
|
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
1 participant
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 #440.
init,generate,stopandreleaseshare one single-threaded executor, sostop()queued behind the generation it was meant to interrupt. TheisGeneratingchecks inside the callback loop could never fire during that generation.isGeneratingis now cleared on the calling thread before queueing — it is@Volatile, so the loop observes the write immediately. AudioTrack teardown stays on the executor, because the generation loop writes to it from there and pausing across threads is the hazard the serialization was protecting against.release()had the same defect.A/B on Pixel 6a
testTTSStopDuringPrefill(120)— stop requested ~120ms into generation:mainOn
maintheStopping TTS generationline lands 1.4s after generation finishes — precisely the no-op #440 describes.Worth recording: that log line appears in both builds. My first A/B checked only for its presence and would have passed either way. Duration and sample count are what actually distinguish them.
Unrelated crash
The app SIGSEGVs afterwards while delivering results to JS — 3.2s after the stop fires, entirely in Hermes frames, and present on
maintoo. That is #436.