Drain audio engine off main queue#655
Conversation
![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: 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 replaces the previous fire-and-forget
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @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.
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.
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 👍 / 👎.
1 participant
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