Guard AVAudioNode.installTap against Objective-C exceptions#23840
Conversation
AVAudioNode.installTap(onBus:bufferSize:format:block:) throws an Objective-C NSException on format mismatch or stale engine state during audio route changes (Bluetooth, AirPods, USB mic). Swift's do/catch cannot intercept NSExceptions — they propagate unhandled and abort(). Add a thin ObjC bridge (VLMPerformWithObjCExceptionHandling) that wraps the call in @try/@catch, converting the fatal crash into a graceful return false. Applied to all installTap call sites: - AudioEngineController.installTapAndStartImpl (macOS dictation + voice) - InputBarView.beginRecording (iOS voice input) Co-Authored-By: tkheyfets <timur@vellum.ai>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69313617fe
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| guard installed else { | ||
| log.error("installTap threw ObjC exception: \(installError?.localizedDescription ?? "unknown")") | ||
| isVoiceOrbExpanded = false | ||
| viewModel.errorText = "Voice input failed. Please try again." | ||
| cleanupRecognition() | ||
| return |
There was a problem hiding this comment.
beginRecording() activates AVAudioSession before installing the tap, but the new guard installed else { ... return } branch only calls cleanupRecognition(). If installTap throws during an audio route change, this path returns without setActive(false), so the .record session (with .duckOthers) can remain active and leave other app audio attenuated until a later successful recording teardown happens. Please deactivate the session in this failure path before returning.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 81a463a. Added try? AVAudioSession.sharedInstance().setActive(false, options: .notifyOthersOnDeactivation) before cleanupRecognition() in the failure path, matching the pattern used in stopRecording() and finishRecordingForAutoStop().
Co-Authored-By: tkheyfets <timur@vellum.ai>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| ), | ||
| .target( | ||
| name: "VellumAssistantShared", | ||
| dependencies: ["ObjCExceptionCatcher"], |
There was a problem hiding this comment.
🚩 VellumAssistantShared depends on ObjCExceptionCatcher but doesn't use it directly
In Package.swift:45, VellumAssistantShared declares a dependency on ObjCExceptionCatcher, but the actual consumers are the iOS app (InputBarView.swift imports it directly) and the macOS library (AudioEngineController.swift imports it directly). VellumAssistantShared itself doesn't appear to import or use ObjCExceptionCatcher. The dependency on VellumAssistantShared is likely added so it transitively reaches the iOS target (which depends on VellumAssistantShared via the XcodeGen project). The iOS project.yml also adds an explicit dependency on ObjCExceptionCatcher (lines 25-26), which is the correct approach. The transitive dependency through VellumAssistantShared is redundant but harmless — worth noting for future cleanup.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Acknowledged — the transitive dependency through VellumAssistantShared is indeed redundant now that the iOS project.yml has an explicit dependency. It's harmless and ensures the target is available regardless of how the dependency graph is resolved, but noted for future cleanup.
Co-Authored-By: tkheyfets <timur@vellum.ai>
|
@codex review |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-Authored-By: tkheyfets <timur@vellum.ai>
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
There was a problem hiding this comment.
🚩 iOS beginRecording() missing AVAudioSession deactivation on channelCount early return
At clients/ios/Views/InputBarView.swift:341-346, the channelCount guard returns early after the AVAudioSession has been activated (lines 323-326), but does not deactivate it with try? AVAudioSession.sharedInstance().setActive(false, ...). This PR correctly adds session deactivation to the two new/modified error paths (installTap exception at line 396, engine start failure at line 474), but misses this pre-existing path. The session remains active until the next recording attempt or app backgrounding. This is a pre-existing issue not introduced by this PR, but given the PR is specifically about hardening error paths with proper cleanup, it may be worth addressing here for consistency.
(Refers to lines 341-346)
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Acknowledged — this is a pre-existing issue (the channelCount guard at line 341 predates this PR). Since the AVAudioSession activation at line 326 is also pre-existing, fixing this early return path is outside the scope of this PR. Worth a follow-up though.
Co-Authored-By: tkheyfets <timur@vellum.ai>
tkheyfets
deleted the
devin/1775517702-fix-installtap-objc-exception-crash
branch
1 participant
Summary
AVAudioNode.installTap(onBus:bufferSize:format:block:)throws an Objective-CNSException(not a SwiftError) on format mismatch or stale engine state during audio route changes (Bluetooth connect/disconnect, AirPods mode switch, USB mic plug/unplug). Swift'sdo/catchcannot interceptNSException— it propagates unhandled through the ObjC runtime and callsabort(), crashing the app.This PR adds a thin ObjC bridge (
VLMPerformWithObjCExceptionHandling) that wraps theinstallTapcall in@try/@catch, converting the fatal crash into a gracefulreturn false. Applied to allinstallTapcall sites:AudioEngineController.installTapAndStartImpl(dictation + OpenAI voice)InputBarView.beginRecordingThe ObjC bridge is packaged as a standalone SPM target (
ObjCExceptionCatcher) depended on by bothVellumAssistantSharedand the iOS Xcode project.Crash report reference: Vellum Staging 0.6.1-staging.4, Thread 2 (
com.vellum.audioEngine.voiceInput),EXC_CRASH (SIGABRT)inAVAudioEngineImpl::InstallTapOnNodeduring concurrent audio hardware state change.Relationship to #23766: That PR (already merged) adds
stop→removeTap→resetbeforeinstallTapto flush stale format caches — a proactive measure. This PR is the defensive complement: if the format mismatch race still occurs despite the reset, the NSException is caught instead of crashing.Updates since initial revision
removeTap(onBus: 0)in the installTap exception handler to prevent a stale partial tap from breaking subsequent recording attempts (matches the pattern in theaudioEngine.start()catch block).AVAudioSession.setActive(false)in both the installTap exception handler and the pre-existingaudioEngine.start()catch block to ensure the.recordsession (with.duckOthers) is deactivated on failure, preventing other apps' audio from staying ducked.Review & Testing Checklist for Human
#import "include/ObjCExceptionCatcher.h"resolves — the relative path in the.mfile depends on SPM's working directory for clang compilation. If it fails, may need#import <ObjCExceptionCatcher/ObjCExceptionCatcher.h>instead.project.ymlnow referencesproduct: ObjCExceptionCatcher; runxcodegenand confirm the Xcode project builds.NS_NOESCAPEbridge block — on iOS, theinstallTaptrailing closure (which is@escaping) is passed inside theNS_NOESCAPEblock toVLMPerformWithObjCExceptionHandling. This should be fine sinceinstallTapretains the tap block internally, but worth a sanity check.Notes
ObjCExceptionCatchertarget is intentionally minimal (one.h, one.m) to keep the ObjC surface area as small as possible.OpenAIVoiceService.swiftalso callsinstallTapAndStartbut routes throughAudioEngineController, so it's already covered by the macOS fix.VellumAssistantShareddeclares a dependency onObjCExceptionCatcherfor transitive availability; the iOSproject.ymlalso adds an explicit dependency. The transitive dep is redundant but harmless.Link to Devin session: https://app.devin.ai/sessions/8724a0e44b3144acb5c06a0241748b1f
Requested by: @tkheyfets