remove tap on engine start failure, fix converter single-use#390
Conversation
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 8080b68 in 17 seconds. Click for details.
- Reviewed
228lines of code in3files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_7La6ANGWDAMQImll
You can customize 
📝 WalkthroughWalkthroughThe changes fix two resource management issues in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
sdk/runanywhere-swift/Sources/RunAnywhere/Features/STT/Services/AudioCaptureManager.swift (1)
197-205: Consider.noDataNowinstead of.endOfStreamfor streaming semantics.
.endOfStreamsignals that the entire audio stream has ended, causing the converter to flush its internal delay buffers (resampler tail). For a continuous tap that processes chunks in sequence,.noDataNowis the correct status — it tells the converter "nothing more for this invocation" without terminating the stream — preserving resampler state across chunk boundaries and avoiding periodic filter-startup artifacts.♻️ Suggested change
if hasProvidedData { - outStatus.pointee = .endOfStream + outStatus.pointee = .noDataNow return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/runanywhere-swift/Sources/RunAnywhere/Features/STT/Services/AudioCaptureManager.swift` around lines 197 - 205, The AVAudioConverter input block (inputBlock) currently sets outStatus.pointee = .endOfStream after sending a single buffer which prematurely terminates the converter and flushes resampler state; change that status to .noDataNow so the converter preserves its internal state between chunked invocations (keep the hasProvidedData/return buffer logic but replace .endOfStream with .noDataNow to signal "no data for this invocation" rather than end-of-stream).sdk/runanywhere-swift/Tests/RunAnywhereTests/AudioCaptureManagerTests.swift (1)
150-156:frameLengthequality is structurally guaranteed, not behavior-derived.Both
firstandsecondget their outputframeCapacityfrom the same deterministic formula (buffer.frameLength × targetRate/sourceRate), makingfirst?.frameLength == second?.frameLengthtrue regardless of converter internal state. The assertion correctly validates thathasProvidedDatais not shared across calls (the PR's actual fix), but it would pass even if the converter had shared mutable state that corrupted audio content. This is acceptable scope for the PR; if full idempotence is a future goal, asserting on sample values (not just frame counts) would be stronger.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/runanywhere-swift/Tests/RunAnywhereTests/AudioCaptureManagerTests.swift` around lines 150 - 156, The test in AudioCaptureManagerTests currently only compares frameLength for two convert(buffer:using:to:) calls which is deterministic and doesn't detect shared mutable state; update the test to assert that the actual audio samples (PCM/sample values) in the returned AVAudioPCMBuffer contents are identical across the two calls (e.g., compare audio data bytes or per-sample floats) and also include an assertion that the buffers are distinct instances (not identical references) to ensure no shared state in the converter/manager; target the convert(buffer:using:to:) call results and the converter instance used in the test when making these assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sdk/runanywhere-swift/Tests/RunAnywhereTests/AudioCaptureManagerTests.swift`:
- Around line 38-45: The assertions are racing with DispatchQueue.main.async
updates inside startRecording/stopRecording: after calling
manager.startRecording and manager.stopRecording you must yield to the main
actor/queue before checking manager.isRecording. Fix by awaiting the main actor
or otherwise yielding the main queue (for example use await MainActor.run { } or
an XCTestExpectation fulfilled from DispatchQueue.main.async) immediately before
each XCTAssertTrue(manager.isRecording) and XCTAssertFalse(manager.isRecording)
so the async updates from startRecording/stopRecording have run.
---
Nitpick comments:
In
`@sdk/runanywhere-swift/Sources/RunAnywhere/Features/STT/Services/AudioCaptureManager.swift`:
- Around line 197-205: The AVAudioConverter input block (inputBlock) currently
sets outStatus.pointee = .endOfStream after sending a single buffer which
prematurely terminates the converter and flushes resampler state; change that
status to .noDataNow so the converter preserves its internal state between
chunked invocations (keep the hasProvidedData/return buffer logic but replace
.endOfStream with .noDataNow to signal "no data for this invocation" rather than
end-of-stream).
In `@sdk/runanywhere-swift/Tests/RunAnywhereTests/AudioCaptureManagerTests.swift`:
- Around line 150-156: The test in AudioCaptureManagerTests currently only
compares frameLength for two convert(buffer:using:to:) calls which is
deterministic and doesn't detect shared mutable state; update the test to assert
that the actual audio samples (PCM/sample values) in the returned
AVAudioPCMBuffer contents are identical across the two calls (e.g., compare
audio data bytes or per-sample floats) and also include an assertion that the
buffers are distinct instances (not identical references) to ensure no shared
state in the converter/manager; target the convert(buffer:using:to:) call
results and the converter instance used in the test when making these
assertions.
sdk/runanywhere-swift/Tests/RunAnywhereTests/AudioCaptureManagerTests.swift
Show resolved
Hide resolved
2 participants
Fixes #198 AudioCaptureManager: resource leak on engine start failure + audio corruption in converter
Description
Fixes two bugs in
AudioCaptureManager(Swift SDK):engine.start()threw after installing the tap oninputNode, the tap was never removed, leaking the tap and leaving the node in a bad state.AVAudioConverterInputBlockinconvert(buffer:using:to:)always returned the same buffer with.haveDataon every call.AVAudioConverter.convert(to:error:withInputFrom:)can invoke the block multiple times, causing duplicated/corrupted audio.Files changed:
sdk/runanywhere-swift/Sources/RunAnywhere/Features/STT/Services/AudioCaptureManager.swift— both fixessdk/runanywhere-swift/Tests/RunAnywhereTests/AudioCaptureManagerTests.swift— new unit testsPackage.swift— new test targetRunAnywhereTestsNote: The issue references
sdk/runanywhere-swift/Modules/ONNXRuntime/Sources/ONNXRuntime/AudioCaptureManager.swift; in this repo the same logic lives insdk/runanywhere-swift/Sources/RunAnywhere/Features/STT/Services/AudioCaptureManager.swift, and that file was updated.Code examples
Fix 1 — Tap cleanup on engine start failure
Before (tap leaked if
engine.start()threw):After (tap removed before rethrowing):
Fix 2 — Converter input block single-use
Before (same buffer returned every call → duplication/corruption):
After (buffer provided once, then
.endOfStream):New test target (
Package.swift):Type of Change
Testing
Local verification (pre-commit on changed files):
Unit tests added (
AudioCaptureManagerTests.swift):testStartRecordingFailureLeavesCleanStatestartRecordingthrows,isRecordingis false and a subsequent start can be attempted (validates clean state / tap cleanup path).testConvertProducesNonDuplicatedOutputconvert()call produces output frame count consistent with one pass (no double feed).testConvertIsIdempotentAcrossCallsconvert()calls with the same buffer each produce valid, same-length output (per-callhasProvidedData).Full Swift test run requires macOS (AVFoundation); CI runs
swift testonmacos-14for this package.Platform-Specific Testing (check all that apply)
Swift SDK / iOS Sample:
PR author environment: Windows + WSL; Swift/AVFoundation tests run in GitHub Actions on macOS.
Kotlin SDK / Android Sample: N/A
Flutter SDK / Flutter Sample: N/A
React Native SDK / React Native Sample: N/A
Web SDK / Web Sample: N/A
Labels
SDKs:
Swift SDK— Changes to Swift SDK (sdk/runanywhere-swift)Sample Apps: None
Checklist
sdk/runanywhere-swift/Tests/RunAnywhereTests/AudioCaptureManagerTests.swift; full run on macOS viaswift test --filter RunAnywhereTestsor the repo’s iOS SDK CI.Summary
engine.start()throws, so no resource leak..endOfStream, so no duplication/corruption.RunAnywhereTeststarget with three tests covering tap-cleanup behavior and converter single-use.convert(buffer:using:to:)isinternalfor testability; no other API changes.Important
Fixes resource leak and audio corruption in
AudioCaptureManagerby ensuring tap removal on engine start failure and single-use converter input block.AudioCaptureManager.swift:inputNodeifengine.start()throws to prevent resource leak.AVAudioConverterInputBlockreturns buffer once, then.endOfStreamto prevent audio duplication/corruption.AudioCaptureManagerTests.swiftwith tests for tap cleanup and converter single-use behavior.RunAnywhereTeststest target inPackage.swift.This description was created by
for 8080b68. You can customize this summary. It will automatically update as commits are pushed.
Greptile Summary
Fixed two critical bugs in
AudioCaptureManagerthat caused resource leaks and audio corruption.Key Changes:
do-catchblock instartRecording()to remove tap oninputNodeifengine.start()throws, preventing leaked taps that leave the node in a bad stateconvert()to usehasProvidedDataflag, ensuring buffer is provided once then returns.endOfStream, preventingAVAudioConverterfrom duplicating audio dataconvert()visibility fromprivatetointernalto enable direct unit testingRunAnywhereTeststarget with 3 comprehensive tests validating both fixesImpact:
Confidence Score: 5/5
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[startRecording called] --> B[Configure AVAudioSession] B --> C[Create AVAudioEngine] C --> D[Install tap on inputNode] D --> E{engine.start<br/>succeeds?} E -->|Yes| F[Set isRecording = true] E -->|No - Before fix| G[Tap leaked on node] E -->|No - After fix| H[Remove tap with<br/>inputNode.removeTap] H --> I[Throw error] G --> J[Node in bad state,<br/>retry fails] I --> K[Clean state,<br/>retry possible] F --> L[Tap callback receives buffer] L --> M[convert called] M --> N{Converter input<br/>block invoked} N -->|Before fix| O[Always return buffer<br/>with .haveData] N -->|After fix| P{hasProvidedData?} P -->|No| Q[Set flag, return<br/>buffer with .haveData] P -->|Yes| R[Return nil<br/>with .endOfStream] O --> S[Converter duplicates<br/>audio data] Q --> T[Single-pass conversion] R --> T S --> U[Corrupted audio output] T --> V[Correct audio output]Last reviewed commit: 8080b68
(3/5) Reply to the agent's comments like "Can you suggest a fix for this @greptileai?" or ask follow-up questions!
Summary by CodeRabbit
Tests
Bug Fixes