Repository navigation
ADFA-5404 | Recover from missing offline language packs - #95
Conversation
…(ADFA-5403) context.services is a per-plugin registry that never holds it, so Voice-to-Code always inserted the raw transcript. Resolve per use, bound generation with a timeout, strip markdown fences, and target the open file's language, not Kotlin.
Resolve the LLM service through getPluginService as well, and drop the cached reference when AI Core unloads. Tune the generation request (system prompt, temperature, maxTokens) instead of scraping fences off the reply, size the timeout to that token budget, and await the future cancellably so plugin teardown unwinds it. Rewrite the fence stripper to handle a lead-in line, an unclosed fence and a one-line fenced reply. Guard logging against an uninitialized context during teardown.
…A-5404) Recognize in the host's configured locale, and on error 12/13 retry once with an installed pack for the same language or online while fetching the missing pack, instead of failing the capture with "unknown error 12".
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
|
I posted on Slack about strings.xml review |
Restore IDLE state and the raw-transcript fallback when AI Core cancels the shared future, release listener/scope/service in deactivate(), and stop the fence stripper from inserting prose (F17) or eating real code (F18, F20).
Teardown conflict resolved: deactivate() now calls teardown(), which releases the capture via endCapture() instead of destroyRecognizer().
Derive STALE_CAPTURE_MS from the generation timeout so the busy guard cannot expire first, and skip the just-failed tag when matching an installed language pack so the retry cannot repeat the same request.
…x/ADFA-5404-language-pack-error-handling
Reset state on activate, reject re-entrant captures, drop trailing prose, accept any fence tag, split null responses from timeouts, clear services.
…x/ADFA-5404-language-pack-error-handling Resolve conflicts in SpeechToTextPlugin.kt and strings.xml: - teardown(): keep 5403's editorService/uiService nulling alongside 5404's endCapture(), which supersedes the bare destroyRecognizer() - startVoiceCapture(): keep 5404's STALE_CAPTURE_MS guard, using 5403's dedicated stt_busy string so stt_error_busy stays reserved for ERROR_RECOGNIZER_BUSY - strings.xml: keep both stt_busy and stt_recovery_failed
usableTag now returns the normalized tag, so an underscore spelling like `es_US` no longer becomes `und` in the fallback toast and retry intent.
itsaky-adfa
left a comment
There was a problem hiding this comment.
Round 4 review. The error-code mapping, the EXTRA_LANGUAGE plumbing and the support-check
recovery all read as sound, and the recognizer lifecycle around supportRecognizer is careful -
the expected guard on destroySupportRecognizer and the PACK_REQUEST_HOLD_MS hold are both
right for the reasons their comments give. What is left is a dead end in the recovery ladder and
the message it produces, a locale-code mismatch, and a prior finding that is closer but not
closed.
Findings by severity
IMPORTANT
SpeechToTextPlugin.kt:557- the installed-pack branch spends the only fallback on a second
offline attempt, so the online retry never runsSpeechToTextPlugin.kt:970- after that fallback the terminal message names a region the user
never chose and claims online recognition failed when it was never triedSpeechToTextPlugin.kt:318(reply on the F28 thread) - the stale guard is measured from the
tap, but the generation timeout it is sized against starts at the transcript
MINOR
SpeechToTextPlugin.kt:665-locale.languagereturns the legacy code, so the same-language
fallback never fires forhe,idoryiSpeechToTextPlugin.kt:420- the shared recognition listener carries no attempt identity
(unproven without a device)
NITPICK - 2 inline, at SpeechToTextPlugin.kt:429 and :1088
Prior rounds
- F28 (busy guard expires before the generation it guards) - partly fixed.
STALE_CAPTURE_MS
is now132_000 + 15_000 = 147_000and does outliveGENERATION_TIMEOUT_SECONDS, so the
arithmetic the original finding pointed at is correct. The window is not closed, though:
captureStartedAtis written only at line 343, on the tap, while the 132 s
withTimeoutOrNullat line 747 starts whenhandleTranscriptruns. Any capture whose
listening plus recognition takes more than 15 s re-opens the same overlap. Separately,
withTimeoutOrNullonly cancels at suspension points, so a backend that blocks inside
service.generateCompletion(...)before returning its future is not bounded by 132 s at all.
Replied on the thread and unresolved it. - F30 (installed-pack branch can retry the exact locale that just failed) - fixed.
usableTagtakesskipRequested(line 661) and the installed-pack call at line 556 passes
skipRequested = true, so the requested tag is filtered out of the candidates before either
match runs. Verified by reading the code at head, not from the reply. Note that the separate
IMPORTANT above is about what happens after this fallback is taken, not a regression of F30. - F31 (recognizer tag spelling goes straight into
Locale.forLanguageTag) - fixed.
normalizeTag(line 677) rewrites_to-and lowercases,usableTagnormalizes the
candidates before matching and returns the normalized tag, and both call sites at lines 558
and 573 build theirLocalefrom that return value.Locale.forLanguageTagcanonicalizes
case, so a lowercase region is fine.
Notes
- Stack. This is the top layer of a two-PR stack:
main<- #93
(fix/ADFA-5403-resolve-llm-from-shared-services) <- #95. Nothing is stacked above it, so
the stack tip is this PR's own head,000cc978, and every claim here was verified against
that commit. No finding was dropped as already-fixed-higher-up, because there is no higher
layer. #93 has to merge first. - Verdict rule. This repo has no
REVIEW.md,CONTRIBUTING.mdor PR template, and
CLAUDE.mdsets no approve/block rule, so the default applied: any confirmed IMPORTANT
blocks. - Not device-verified. Per
CLAUDE.md, a greenassemblePluginis not verification for
these plugins, and every finding here is from reading the code at head. The installed-pack
dead end in particular is worth exercising on a device with a stale entry in
installedOnDeviceLanguages.
itsaky-adfa
left a comment
There was a problem hiding this comment.
Blocking on the three IMPORTANT findings from the inline review. In priority order:
-
The installed-pack branch is a dead end (
SpeechToTextPlugin.kt:557). It spends the capture's singlelanguageFallbackSpentbudget on a second offline attempt, so when a staleinstalledOnDeviceLanguagesentry fails, the online retry this PR exists for never runs. Track the budget per strategy - one offline-region fallback plus one online retry. -
The terminal message after that fallback is wrong twice (
SpeechToTextPlugin.kt:970).languageLabel()readsactiveLocale, which the fallback overwrote, so the user is named a region they never chose and sent to install a pack for it - and the string claims online recognition failed when that path ran withpreferOffline = true. -
F28 is partly fixed, so I reopened its thread. The 147 s constant does outlive the 132 s generation, but
captureStartedAtis set at the tap while the timeout starts at the transcript, so the 15 s headroom has to absorb the whole listening phase.
The two MINOR and two NITPICK comments are not blockers. The recognizer lifecycle around supportRecognizer reads as careful and F30 and F31 are genuinely closed - I verified both at head rather than from the replies.
Note that this PR is stacked on #93, which has to merge first.
Drop an unlisted fence tag from the opening line, limit prose filtering to the reply's leading and trailing runs, guard the cached LLM router against teardown, and widen the IDE window to 26.29-26.99 to match the APIs used.
…x/ADFA-5404-language-pack-error-handling Resolve teardown() conflict: keep 5403's synchronized(serviceLock) cancel-and-clear and end with 5404's endCapture(), which subsumes destroyRecognizer().
Budget language recovery per strategy so a failed offline fallback still reaches the network, name the requested locale in user-facing messages, latch recognizer callbacks per attempt, and tag generations by capture id.
itsaky-adfa
left a comment
There was a problem hiding this comment.
Re-review of e2df93b. All seven findings from my 2026-09-14 review verify as fixed at head, so the REQUEST_CHANGES standing from that round no longer has a basis. Three new findings below, one of them blocking.
Severity index
IMPORTANT
SpeechToTextPlugin.kt:285- teardown cannot stop an in-flight support check, so the mic can start on a disabled plugin
MINOR
SpeechToTextPlugin.kt:720- the pack-download hold sharesrecoveryToken, so the next tap orphans the support recognizer
NITPICK - 1 inline, not listed
Re-check of the previous rounds
Read at head, not taken from the replies.
- Installed-pack branch was a dead end (
#discussion_r4006795555) - fixed.languageFallbackSpentis nowofflineFallbackSpent+onlineRetrySpent, each set byrestartRecognitionfor the route that picked the attempt (line 771). Traced the case I named: the installed-pack retry errors again,recoverFromLanguageErrorfalls to its second branch,retryOnline()runs. The network is reached. languageLabel()named the fallback locale (#discussion_r4006795567) - fixed.requestedLocaleis set once per capture (line 394) andlanguageLabel()reads it (line 1130).retryOnline()asks for it too.stt_error_language_unavailableis now only reachable withonlineRetrySpenttrue, and that flag is set only byrestartRecognition(preferOffline = false), so the "online recognition didn't work either" wording is earned on every path that shows it.locale.languagelegacy ISO-639 codes (#discussion_r4006795577) - fixed.val language = wanted.substringBefore('-')(line 779).- Shared listener carried no attempt identity (
#discussion_r4006795599) - fixed for the case I raised.recognitionListener(attempt)is built per attempt and installed with++recognitionAttempt(line 422), with aclaim()latch. I checked the rest of the interface: the other seven overrides are empty, so the two terminal callbacks are the whole surface. The teardown variant of the same problem is the IMPORTANT above, and it is a different hole, not a regression of this one. - minSdk comment contradicted
build.gradle.kts(#discussion_r4006795606) - fixed. The comment now states why the runtime check is deliberate rather than naming a floor the build file disagrees with. - 1.2 s support timeout (
#discussion_r4006795610) - addressed. Raised to 4 s with the tradeoff in the KDoc. The substance of the nit was "measure a cold bind" and there is still no measurement; the direction is right and only device testing settles the value. - Stale-capture window (
#discussion_r4006799691) - fixed, all three parts.captureStartedAtrestarts at PROCESSING (line 487) so the 15 s headroom covers the generation alone;captureIdgates the insertion (line 812) and thefinallyspinner reset (line 829);generationJob?.cancel()and the token clear both run beforecaptureId++(lines 388-390), and the cancelled coroutine'sfinallyreaches main by a post, so it always sees the incremented id. The unboundedservice.generateCompletioncall is still not bounded bywithTimeoutOrNull, but the capture id keeps its result out of the capture that replaced it, which was the harm; the comment at line 858 records the limitation honestly.
Every prior thread was already resolved by the author, and nothing here reopens one.
Considered and not filed
The download recognizer held by requestLanguagePack overlaps the online retry by ~4.6 s on the same RecognitionService, which would matter if it made the retry return ERROR_RECOGNIZER_BUSY - the one outcome this PR exists to prevent, on its main new path. I did not file it: onTriggerModelDownload is a separate RecognitionService entry point from the listening session and should not occupy mCurrentCallback, so the premise is probably wrong, and I cannot settle it by reading. It is worth one look on device while you are exercising the download path anyway.
Verification
../../gradlew assemblePlugin succeeds (exit 0). Per this repo's CLAUDE.md that is not verification - the real check is installing the .cgp through the Plugin Manager and exercising it. I had no device in this session, so nothing here was confirmed on hardware. The language-pack paths are exactly the ones that need it: checkRecognitionSupport cold-bind timing against SUPPORT_TIMEOUT_MS, whether triggerModelDownload really queues behind the 5 s hold, and whether the recognizer double-delivers the way the code now assumes.
Rules applied
This repo has no written approve/request-changes rule - CLAUDE.md covers plugin structure and verification, not review outcomes - so the default applied: IMPORTANT blocks, MINOR does not.
Stack
Two-layer stack: #93 (based on main), then this PR. #95 is the tip, so the review head and the verification ref are the same commit, e2df93b. No finding was dropped as already-fixed-higher-up, and nothing filed here belongs to #93.
Stop an inline-closed fence and a multi-word info string from reaching the file, keep prose-shaped languages out of the prose filter and log any drop, let a tap during RECORDING restart the capture, and close the forgetLlmService cache race with an epoch.
…x/ADFA-5404-language-pack-error-handling Both branches rewrote the re-entrancy guard in startVoiceCapture(). Keep both intents: PROCESSING is refused until the capture goes stale (5404's escape from a wedged generation), while a tap during RECORDING falls through and restarts the capture (5403's, since nothing bounds a recognizer that never delivers a terminal callback). The fall-through is safe because the post-guard block already cancels the recovery callbacks, the generation job, and bumps the capture id.
Bump recognitionAttempt in endCapture() and re-check it in onSupportResult, so a support answer or recognizer callback already queued on the main looper cannot start the mic on a torn-down capture.
itsaky-adfa
left a comment
There was a problem hiding this comment.
Fifth-pass re-review of the language-pack recovery. Built clean at 1946a9f (../../gradlew assemblePlugin -> speech-to-text.cgp, 9.5 MB); not device-verified - no emulator was reachable in this session, and per CLAUDE.md a green build is not verification for this repo. Findings 1, 4, 5 and 6 below are device-observable and would need an install to settle.
Stack. This is the tip of a two-PR stack: #93 (fix/ADFA-5403-..., based on main) sits below it, nothing above. Tip and review head are the same commit, 1946a9f, so every claim was checked against the code that would actually reach main. Nothing was dropped as already-fixed-higher-up.
Verdict rule. This repo has no written approve/request-changes rule - CLAUDE.md covers plugin conventions and the /plugin-review submission rubric, not merge gating - so the default applied: MINOR does not block.
Severity index
MINOR
SpeechToTextPlugin.kt:1191- the host locale's-u-extension subtags reachEXTRA_LANGUAGE, the tag match and the toastsSpeechToTextPlugin.kt:380- a RECORDING tap restarts the recognizer with no teardown delaySpeechToTextPlugin.kt:381- the stale-capture guard uses the wall clock, not a monotonic oneSpeechToTextPlugin.kt:618- the next capture's support check cancels the pack download inside its hold windowSpeechToTextPlugin.kt:642- a stale support answer destroys a newer capture's recognizerSpeechToTextPlugin.kt:660- the two support error codes are swapped in the commentdocs/index.html:69- says taps are ignored during a capture; they restart it while listening
NITPICK - 1 inline, not listed
Previous rounds
All twelve earlier threads re-checked by reading the code at 1946a9f, not by taking the replies:
- F28 / busy guard expiring before the generation - fixed.
STALE_CAPTURE_MSisGENERATION_TIMEOUT_SECONDS * MILLIS_PER_SECOND + 15_000= 147 s against the 132 s timeout,captureStartedAtrestarts at PROCESSING (line 508),captureIdgates both the insertion (line 845) and thefinally'ssetState(line 862), and a tap that passes the guard clearsrecoveryTokenand cancelsgenerationJob(lines 409-410). The wall-clock choice is a new finding above, not a regression of this one. - F30 / installed-pack branch retrying the failed locale - fixed.
usableTag(..., skipRequested = true)at line 690. - F31 / raw tag into
Locale.forLanguageTag- fixed.usableTagnormalizes candidates and returns the normalized tag (lines 811-814). - Installed-pack dead end - fixed.
offlineFallbackSpent/onlineRetrySpentare separate budgets, spent inrestartRecognition(line 782), so a second error 13 always reaches the online branch. - Error message naming the fallback language - fixed.
requestedLocaleis set once per capture (line 416) andlanguageLabel()reads it (line 1205);retryOnline()asks for it too. - Legacy ISO-639 codes - fixed.
val language = wanted.substringBefore('-')at line 810. - Shared listener with no attempt identity - fixed.
recognitionListener(attempt)plus theclaim()latch (lines 484-492). minSdkcomment - fixed, comment now states the loader gates onplugin.min_ide_version.- Support timeout too short - fixed, 4 s with the tradeoff in the KDoc.
- Support answer outliving a teardown - fixed.
endCapture()bumpsrecognitionAttempt(line 306) andonSupportResultre-checks it (line 646). ThedestroySupportRecognizer()ordering around that check is the new finding at line 642. - Pack release cancelled by the next tap - fixed, the
PACK_REQUEST_HOLD_MSpost is untokened (line 751). The sibling path at line 618 that still cancels it is the new finding above. skipRequestedexact-match KDoc - fixed (line 796).
No prior finding regressed and none was left unaddressed.
Monotonic stale-capture clock, a RECORDING tap routed through the restart delay, a separate pack-download recognizer, and corrected 14/15 error names.
…x/ADFA-5404-language-pack-error-handling
…x/ADFA-5404-language-pack-error-handling
itsaky-adfa
left a comment
There was a problem hiding this comment.
Fifth round on this PR. Findings are all MINOR or below; the computed verdict was COMMENT, and an APPROVE follows this review at the reviewer's explicit direction, since MINOR is defined as safe to merge. This repo has no written approve/request-changes rule (no REVIEW.md, CONTRIBUTING.md or PR template), so the default applied.
Stack: this PR sits on fix/ADFA-5403-... (#93), which is based on main. Nothing is stacked above it, so this PR is the tip and every claim below was verified against its own head, 71619fe.
assemblePlugin is clean at head (isolated worktree, minSdk 33). That is a compile check, not verification - none of this was exercised on a device in this session, and the recovery paths here are device behaviour.
Findings
MINOR
assets/docs/index.html:64- the page promises the words already spoken still land; the retry discards themSpeechToTextPlugin.kt:314- the teardown attempt bump is unsynchronized and may not run on mainSpeechToTextPlugin.kt:426- no route out of a RECORDING capture that never settlesSpeechToTextPlugin.kt:453-preferOffline = falsedoes not force the network, but a string says it didSpeechToTextPlugin.kt:476- the recovery prompt can appear after the retry is already listeningSpeechToTextPlugin.kt:741- a silently failed pack request still toasts "fetching it for next time"SpeechToTextPlugin.kt:1234- errors 10 and 11 still fall through to the generic wording
NITPICK - 1 inline, not listed
Previous rounds
All 21 findings from the four earlier rounds are fixed, checked by reading head rather than the replies:
- F28, stale guard shorter than the generation - fixed.
STALE_CAPTURE_MSis now derived fromGENERATION_TIMEOUT_SECONDS(147 s vs 132 s),captureStartedAtrestarts at PROCESSING (line 535) so the headroom covers the generation alone,captureIdgates both the insertion and thefinally'ssetState, and a tap that passes the guard clearsrecoveryTokenand cancelsgenerationJob. - F30, installed-pack branch retrying the locale that just failed - fixed by
skipRequested. - F31, underscore tags reaching
Locale.forLanguageTagasund- fixed;usableTagnormalizes candidates and returns the normalized tag. - Installed-pack dead end - fixed.
offlineFallbackSpent/onlineRetrySpentare separate budgets, so a failed offline fallback still reaches the network. I traced all four routes throughrecoverFromSupport: none can reachdescribeErrorwithout the network having been attempted. - Wrong language named after a fallback - fixed;
requestedLocaleis set once per capture andlanguageLabel()/retryOnline()both read it. Locale.getLanguage()legacy codes (iw/in/ji) - fixed; the subtag comes from the normalized tag.- Shared listener with no attempt identity - fixed;
recognitionListener(attempt)plus theclaim()latch. minSdkcomment vs the SDK_INT guard - fixed; the comment now says why the guard is deliberate.- 1.2 s support timeout - raised to 4 s with the tradeoff recorded.
- Support answer reaching a disabled plugin - fixed;
endCapture()bumps the attempt andonSupportResultre-checks it. (Finding at line 314 above is about that bump's thread safety, not about the check being absent.) - Pack release cancelled by the next tap - fixed twice over: its own
packRecognizerfield, and the hold is posted untokened. - The three
checkLanguageSupportreleases tearing down a newer recognizer - fixed; all three passexpected. - Swapped 14/15 in the support-error comment - fixed; 14 is named
ERROR_CANNOT_CHECK_SUPPORT, which matchesandroid.jar. -u-extension subtags in tags and toasts - fixed byclearExtensions().++recognitionAttemptinside the argument - fixed; the bump is avalbefore thetry.- Docs claiming taps are ignored - fixed for the tap behaviour. The line 64 finding above is a different claim on the same page.
One earlier suggestion I assessed and am not raising: recoverFromSupport looks up supportedOnDeviceLanguages without skipRequested while the installed lookup uses it. The asymmetry is right - re-issuing the identical offline request is futile, but re-downloading a pack that errors 13 is a plausible repair, which the comment at line 727 already argues.
itsaky-adfa
left a comment
There was a problem hiding this comment.
Approving. Everything raised across the previous four rounds is fixed at head, and the eight findings in the review above are MINOR or below - real, but none of them blocks a merge.
Nothing needs to happen before this lands. Worth picking up after, roughly in order: the help page at index.html:64 promising the spoken words survive the online retry, since that one sends a user to wait for text that is not coming; the silent pack-request failure at SpeechToTextPlugin.kt:741; and errors 10 and 11 still falling through to the generic wording this PR set out to remove.
One caveat on coverage: assemblePlugin is clean, but nothing here was exercised on a device in this session, and language-pack recovery is device behaviour. The recording in the description covers the EN/ES path; the branches I could not reach - a corrupt installed pack, a pre-33 host, a support check that times out - are read from the code only.
Volatile attempt counter, cancel a capture wedged before the mic opens, reuse the last toast slot, report failed pack requests, handle errors 10/11, fix locale labels.
…e-pack-error-handling # Conflicts: # plugins/Speech-to-Text/src/main/kotlin/com/itsaky/androidide/plugins/stt/SpeechToTextPlugin.kt # plugins/Speech-to-Text/src/main/res/values/strings.xml
Description
Replaces the generic "unknown error 12" with clear, actionable UI messages that display the specific missing Locale. Implements an automatic single online retry when an offline language pack is missing, ensuring the user's dictation session stays alive without being prematurely aborted.
Details
ERROR_LANGUAGE_UNAVAILABLEandERROR_LANGUAGE_NOT_SUPPORTEDinSpeechToTextPlugin.kt.preferOfflineand setEXTRA_LANGUAGEto support a single online retry attempt.strings.xmlto inform the user of the exact failing language and the fallback network attempt.index.htmldocumentation to reflect the new language pack recovery behavior.Prompt EN:
Function that reverses a stringPrompt ES:
Función que reversa un stringdocument_4976756881377724759.mp4
Ticket
ADFA-5404
Parent: ADFA-5402
Observation
The plugin now queries
checkRecognitionSupporton API 33+ devices to find regional fallback packs or trigger downloads, defaulting to a direct network retry on older SDKs.