Description
Two concurrency defects in AudioRecorderManager, found during Codex review of #464. Both are pre-existing and in paths that PR does not touch, so they are filed rather than folded into it.
1. Device-change teardown can restart a dead recording
handleDeviceChange checks _isRecording, releases the lock, sleeps, and later initializes and starts a new AudioRecord (around lines 299-340). A concurrent stopRecording() can complete during that gap. Nothing rechecks state before initialization or start, so the path can leave a running AudioRecord with _isRecording == false and no recording thread consuming it.
2. Preparation and start are not serialized
initializeAudioRecord calls discardFailedAttempt(), which releases and nulls both recorders. prepareRecording() runs on an IO coroutine and sets isPrepared only at the end, so a concurrent prepareRecording() / startRecording() pair can release each other's recorders, replace an in-use AudioRecord, or allocate after OnDestroy has already cleaned up.
Environment
- expo-audio-studio version: workspace (
packages/audio-studio)
- Platform & OS version: Android, any
Why this is filed separately
#464 fixed the reclamation-ownership defects on the single-threaded paths (who releases the compressed recorder, and when). These two are genuine races between callers, and a point fix on either would be guesswork without a way to exercise the interleaving.
That is the same conclusion #449 reached from the other direction: these paths need an injectable AudioRecord / MediaRecorder factory so ownership and lifecycle can be tested deterministically. Device validation cannot reach them — both fault injections attempted during #464 validation were rejected during config parsing, before any recorder was allocated.
Suggested
Introduce the factory seam first, then fix these with tests that actually reproduce the interleaving.
Related
Description
Two concurrency defects in
AudioRecorderManager, found during Codex review of #464. Both are pre-existing and in paths that PR does not touch, so they are filed rather than folded into it.1. Device-change teardown can restart a dead recording
handleDeviceChangechecks_isRecording, releases the lock, sleeps, and later initializes and starts a newAudioRecord(around lines 299-340). A concurrentstopRecording()can complete during that gap. Nothing rechecks state before initialization or start, so the path can leave a runningAudioRecordwith_isRecording == falseand no recording thread consuming it.2. Preparation and start are not serialized
initializeAudioRecordcallsdiscardFailedAttempt(), which releases and nulls both recorders.prepareRecording()runs on an IO coroutine and setsisPreparedonly at the end, so a concurrentprepareRecording()/startRecording()pair can release each other's recorders, replace an in-useAudioRecord, or allocate afterOnDestroyhas already cleaned up.Environment
packages/audio-studio)Why this is filed separately
#464 fixed the reclamation-ownership defects on the single-threaded paths (who releases the compressed recorder, and when). These two are genuine races between callers, and a point fix on either would be guesswork without a way to exercise the interleaving.
That is the same conclusion #449 reached from the other direction: these paths need an injectable
AudioRecord/MediaRecorderfactory so ownership and lifecycle can be tested deterministically. Device validation cannot reach them — both fault injections attempted during #464 validation were rejected during config parsing, before any recorder was allocated.Suggested
Introduce the factory seam first, then fix these with tests that actually reproduce the interleaving.
Related