Drain audio engine off main queue - #655
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5802b15e8
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Greptile SummaryThis PR fixes a crash that occurred when audio events were delivered through a destroyed
|
| @testable import FluidVoice_Debug | ||
| import Foundation | ||
| import XCTest | ||
|
|
||
| final class AudioEngineRetirementDrainTests: XCTestCase { | ||
| func testReleaseAndWaitCompletesAfterOffMainDeinit() async throws { | ||
| let drain = AudioEngineRetirementDrain(label: "test.audio-engine-retirement.single") | ||
| let recorder = DeinitRecorder() | ||
| var probe: DeinitProbe? = DeinitProbe(id: 1, recorder: recorder) | ||
| let token = AudioEngineRetirementToken(try XCTUnwrap(probe)) | ||
| probe = nil | ||
|
|
||
| await drain.releaseAndWait(token) | ||
|
|
||
| XCTAssertEqual( | ||
| recorder.events, | ||
| [DeinitEvent(id: 1, occurredOnMainThread: false)] | ||
| ) | ||
| } | ||
|
|
||
| func testAwaitedReleaseRunsAfterPreviouslyScheduledRelease() async throws { | ||
| let drain = AudioEngineRetirementDrain(label: "test.audio-engine-retirement.serial") | ||
| let recorder = DeinitRecorder() | ||
| var first: DeinitProbe? = DeinitProbe(id: 1, recorder: recorder) | ||
| var second: DeinitProbe? = DeinitProbe(id: 2, recorder: recorder) | ||
| let firstToken = AudioEngineRetirementToken(try XCTUnwrap(first)) | ||
| let secondToken = AudioEngineRetirementToken(try XCTUnwrap(second)) | ||
| first = nil | ||
| second = nil | ||
|
|
||
| drain.schedule(firstToken) | ||
| await drain.releaseAndWait(secondToken) | ||
|
|
||
| XCTAssertEqual( | ||
| recorder.events, | ||
| [ | ||
| DeinitEvent(id: 1, occurredOnMainThread: false), | ||
| DeinitEvent(id: 2, occurredOnMainThread: false), | ||
| ] | ||
| ) | ||
| } |
There was a problem hiding this comment.
Missing coverage for
waitForScheduledReleases barrier
The two tests cover releaseAndWait alone and schedule + releaseAndWait ordering, but waitForScheduledReleases — the barrier used in startPreferredAudioCapture, startCompatibilityAudioCapture, and cancelAudioRouteRecoveryAndWait — is not exercised at all. A test that calls drain.schedule(token) then awaits drain.waitForScheduledReleases() and asserts the engine was released before the barrier returned would give confidence that the most-travelled production path (capture start after a fire-and-forget retirement) actually serializes correctly.
Prompt To Fix With AI
This is a comment left during a code review.
Path: Tests/FluidDictationIntegrationTests/AudioEngineRetirementDrainTests.swift
Line: 1-41
Comment:
**Missing coverage for `waitForScheduledReleases` barrier**
The two tests cover `releaseAndWait` alone and `schedule` + `releaseAndWait` ordering, but `waitForScheduledReleases` — the barrier used in `startPreferredAudioCapture`, `startCompatibilityAudioCapture`, and `cancelAudioRouteRecoveryAndWait` — is not exercised at all. A test that calls `drain.schedule(token)` then awaits `drain.waitForScheduledReleases()` and asserts the engine was released before the barrier returned would give confidence that the most-travelled production path (capture start after a fire-and-forget retirement) actually serializes correctly.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 336c5351f9
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08bc0eec64
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| self.audioRouteRecoveryTask?.cancel() | ||
| self.audioRouteRecoveryTask = nil | ||
| self.isRecoveringAudioRoute = false | ||
| await self.cancelAudioRouteRecoveryAndWait() |
There was a problem hiding this comment.
Treat startup as active in automatic hotkey routing
In automatic activation mode, this new suspension leaves isRunning false long enough for a second, different mode shortcut to take the isRunning == false branches in GlobalHotkeyManager (for example handlePromptModeKeyDown and the command/rewrite equivalents). Those branches invoke beginDictationRecording, which changes the active mode and prompt selection before its guard !asr.isRunning rejects the duplicate engine start. Consequently, a quick mode-key press while route recovery is draining can capture audio started by the first shortcut but finalize/output it using the second mode's configuration; the automatic paths need to consider isStarting just as the updated toggle paths do.
Useful? React with 👍 / 👎.
|
@greptileai review |
…v#655 altic-dev#655 ("Drain audio engine off main queue") landed while this branch was open and reworked the same capture-startup path this branch guards, so the two collided in `ASRService.startPreferredAudioCapture()`. Both sides are additive and orthogonal — altic-dev#655 serialises AVAudioEngine teardown against startup, this branch refuses to start when the pinned microphone cannot be honoured — so the resolution keeps both: - `startPreferredAudioCapture()` becomes `async throws` and awaits `audioEngineRetirementDrain.waitForScheduledReleases()`, per altic-dev#655. - The FluidVoice-only guards (missing preferred device, AVAudioEngine fallback) are retained unchanged. - The missing-device check runs *before* the drain await: it is a cheap precondition that throws, and there is no reason to wait on a teardown for a capture that is about to be refused. - The compatibility-capture call becomes `try await`. Both callers of `startPreferredAudioCapture()` already awaited it on main's side, so no further propagation was needed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SG5paEAboAJeNcbazuzqrQ
Description
Right now, whenever audio input device switches the AVAudioEngine is destroyed and created. However, sometimes, the audio events can be be sent through the destroyed engine which the ASRService doesn't realize. This results in a crash. This change fixes this by making the destruction/creation sequential and also waiting for a quiet period (if it doesn't happen instantly) before switching the engine. This should prevent crashses
Type of Change
Related Issue or Discussion
#640, #644, #636 and #651
Testing
swiftlint --strict --config .swiftlint.yml Sourcesswiftformat --config .swiftformat SourcesScreenshots / Video
Attempted transcription and also switching between audio input devices to make sure it works as expected