fix(capture): prevent double-recording when system audio and device recording run concurrently (#3244)#6353
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: 9f08b26c22
ℹ️ 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 adds a synchronous mutex ( Confidence Score: 4/5Safe to merge after fixing the failure-path state leak in streamSystemAudioRecording The core mutex approach is correct and well-tested. One P1 defect: if _initiateWebsocket() returns with a null socket, state is unconditionally advanced to systemAudioRecord, permanently blocking BLE recording with no user-accessible recovery path. app/lib/providers/capture_provider.dart lines 1039-1046 Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[streamSystemAudioRecording] --> B{isDesktop?}
B -- No --> Z[return]
B -- Yes --> C[SET initialising SYNC]
C --> D{_recordingDevice != null?}
D -- Yes --> E[cleanupCurrentState + socket.stop]
D -- No --> F[resetStateVariables]
E --> F
F --> G[_initiateWebsocket]
G --> H{_socket == null?}
H -- Yes --> I[_startKeepAliveServices + return\n⚠️ state unconditionally set to systemAudioRecord]
H -- No --> J[SET systemAudioRecord]
I --> J
K[streamDeviceRecording] --> L{isDesktop?}
L -- No --> M[proceed]
L -- Yes --> N{state == systemAudioRecord OR initialising?}
N -- Yes --> O[return early]
N -- No --> M
P[keepAlive timer] --> Q{isDesktop?}
Q -- No --> R[reconnect]
Q -- Yes --> S{state == systemAudioRecord OR initialising?}
S -- Yes --> T[cancel timer]
S -- No --> R
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR addresses a desktop race condition where system audio capture and BLE/device recording could start concurrently, resulting in two WebSocket connections and duplicate transcription streams.
Changes:
- Adds a desktop-only guard/mutex by setting
RecordingState.initialisingsynchronously at the start ofstreamSystemAudioRecording(). - Prevents device recording and keep-alive reconnect from starting while system audio is active/initialising on desktop.
- Introduces
PlatformService.isDesktopand adds unit tests covering the new guard behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| app/lib/providers/capture_provider.dart | Adds system-audio start/stop flow and desktop guards to prevent concurrent device/system-audio sockets. |
| app/lib/utils/platform/platform_service.dart | Adds isDesktop helper for centralized desktop gating. |
| app/test/providers/capture_provider_test.dart | Adds tests validating early-return guards and the synchronous “initialising” mutex property. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e50c1aeb70
ℹ️ 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".
eulicesl
force-pushed
the
fix/3244-double-recording-desktop
branch
from
d631394 to
8d43c59
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d43c59f9d
ℹ️ 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".
eulicesl
force-pushed
the
fix/3244-double-recording-desktop
branch
from
8d43c59 to
fbabfec
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fbabfec50b
ℹ️ 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".
…ystem-audio failure - streamDeviceRecording: add a third guard after await _resetState() (the device-init await) so a concurrent system-audio start that queued during that suspension bails out and cleans up before it can leave a competing device WebSocket open. - streamSystemAudioRecording: defer BLE teardown until after _initiateWebsocket confirms the desktop socket is up. On failure (socket null or throw), explicitly stop any partial socket, reset RecordingState, and re-establish device streaming via _resetState() instead of silently dropping to stop. - _startKeepAliveServices: add desktop guard after await _getAudioCodec() so a system-audio path that began initialising during the codec fetch cannot race through to _initiateWebsocket and open a competing BLE websocket. Fixes review comments on PR BasedHardware#6353.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d77ebd538
ℹ️ 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: a6375d818d
ℹ️ 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".
…g stream (BasedHardware#3244) Co-Authored-By: Eulices Lopez <leulices@gmail.com>
…ystem-audio failure - streamDeviceRecording: add a third guard after await _resetState() (the device-init await) so a concurrent system-audio start that queued during that suspension bails out and cleans up before it can leave a competing device WebSocket open. - streamSystemAudioRecording: defer BLE teardown until after _initiateWebsocket confirms the desktop socket is up. On failure (socket null or throw), explicitly stop any partial socket, reset RecordingState, and re-establish device streaming via _resetState() instead of silently dropping to stop. - _startKeepAliveServices: add desktop guard after await _getAudioCodec() so a system-audio path that began initialising during the codec fetch cannot race through to _initiateWebsocket and open a competing BLE websocket. Fixes review comments on PR BasedHardware#6353.
…d socket in recovery path - Add _desktopHandoffInProgress flag so onClosed() skips the initialising->stop reset when the BLE close is an intentional desktop-audio handoff, not a real disconnection. - Capture socketBeforeDesktopInit before _initiateWebsocket so the recovery helper stops the OLD BLE socket, not the newly-opened desktop socket.
eulicesl
force-pushed
the
fix/3244-double-recording-desktop
branch
from
d9519e4 to
526649f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 526649fa60
ℹ️ 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".
| if (PlatformService.isDesktop && | ||
| (recordingState == RecordingState.systemAudioRecord || | ||
| (recordingState == RecordingState.initialising && !_desktopHandoffInProgress))) { | ||
| updateRecordingState(RecordingState.stop); |
There was a problem hiding this comment.
This onClosed branch can clear RecordingState.initialising to stop while streamSystemAudioRecording() is still in flight, which re-opens the race this patch is trying to prevent. Fresh evidence: _initiateWebsocket() delegates to SocketServicePool.socket(), and that path unconditionally stops the previous socket before creating the desktop socket; that stop triggers onClosed() while _desktopHandoffInProgress is still false, so the mutex is released before handoff completion.
Useful? React with 👍 / 👎.
| await _cleanupCurrentState(); | ||
| await _socket?.stop(reason: 'desktop handoff — aborting device init'); |
There was a problem hiding this comment.
In this race-abort branch, stopping the shared _socket can terminate the newly opened desktop conversation instead of the device socket, because _socket may have been overwritten by a concurrent streamSystemAudioRecording() while streamDeviceRecording() was awaiting _resetState(). Fresh evidence: this explicit stop was added in the post-_resetState() guard, but the code does not retain a device-socket handle, so the wrong connection can be closed under the exact concurrent-start scenario being fixed.
Useful? React with 👍 / 👎.
Summary
RecordingState.initialising) set before the firstawaitinstreamSystemAudioRecording;streamDeviceRecordingand_startKeepAliveServicesnow check forsystemAudioRecordandinitialisingstates and return earlyLinked Issue / Bounty
Root Cause (bug fixes only)
streamSystemAudioRecordingsetRecordingState.systemAudioRecordonly after severalawaitcalls, leaving a window where concurrent callers saw the state as idle and could start a competing device recording sessionHow It Works
PlatformService.isDesktopgetter added — gates all desktop-only recording paths with a single readable checkstreamSystemAudioRecording()— setsRecordingState.initialisingsynchronously before the firstawait, acting as a mutex under Dart's single-threaded event loop; tears down any active BLE device inline (not viastopStreamDeviceRecording) to avoid widening the race window with an extra async hopstreamDeviceRecording()— returns early ifsystemAudioRecordorinitialisingis active on desktop_startKeepAliveServices()— cancels the keep-alive timer and returns early for the same two states, preventing a BLE reconnect event from opening a device WebSocket while system audio is runningstopSystemAudioRecording()— clean teardown that resets state and restarts keep-alive servicesChange Type
Scope
Security Impact
Testing
capture_provider.dartapp/test/providers/capture_provider_test.dart— 3 new tests covering:streamDeviceRecordingskips whensystemAudioRecordis activestreamDeviceRecordingskips wheninitialisingis activestreamSystemAudioRecordingsets state toinitialisingsynchronously before anyawait(verifying mutex property)Human Verification
awaitis guaranteed atomic under single-threaded model)initialisingstate as the guard (not justsystemAudioRecord)Evidence
capture_provider.dartDocs
Performance, Privacy, and Reliability
Migration / Backward Compatibility
Risks and Mitigations
initialisingguard prevents device recording from starting during the system audio init window — if system audio init fails partway through, the state must be reset or device recording is permanently blockedstreamSystemAudioRecordingresets state toRecordingState.initialising's predecessor on any failure path;stopSystemAudioRecordingalso resets state unconditionallystopSystemAudioRecordingis not yet called from a UI entry point — system audio sessions can only be stopped programmatically in the current implementationAI Disclosure