fix(audio): sequence engine rebuild behind retired-engine teardown to stop launch freeze (#620)#621
Conversation
…ltic-dev#620) An idle route change retires the prewarmed AVAudioEngine — whose dealloc always drains on a background queue since the altic-dev#542 fix — and immediately builds its replacement on the main thread. -[AVAudioEngine dealloc] and inputNode/prepare() on a fresh engine serialize inside the HAL, so teardown and bring-up of the two engines can interleave: the main thread parks in a workloop sync wait behind a servicer stuck in a condition wait, and the app freezes for good. The spindump in altic-dev#620 shows this signature ~15s after launch, when Bluetooth/default devices settle and fire route-change bursts. - Give retired engines one serial drain queue and an onDrained hook so bring-up can be ordered strictly after dealloc completes. - Idle route change: rebuild in the drain completion, with a generation counter so a burst of route changes collapses into a single rebuild of the newest configuration. - Mid-recording recovery: cooperatively await the drain (bounded at 5s) before rebuilding capture; if the teardown wedges, stop the recording cleanly instead of racing a new engine against it, and re-check isRunning after the suspension.
![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: cf995a71a9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // bursts into one rebuild of the newest configuration. | ||
| self.idleEngineRebuildGeneration &+= 1 | ||
| let generation = self.idleEngineRebuildGeneration | ||
| self.retireAudioEngine(reason: "idle_route_change:\(reason)", onDrained: { [weak self] in |
There was a problem hiding this comment.
When an idle route change retires a warm engine, this only sequences the background prewarmAudioEngineIfPossible behind onDrained, but engineStorage is cleared immediately. If the user starts recording before the drain callback fires, startPreferredAudioCapture() can still fall through to configureSession()/startEngine() and instantiate/prepare a new AVAudioEngine while the retired one is deallocating, recreating the HAL overlap this change is trying to avoid. Consider tracking the pending drain and awaiting it from the start path as well as from idle prewarm.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed and fixed in 7679bb1 — start() now awaits awaitPendingEngineDrains() before startPreferredAudioCapture() whenever a drain is pending (the common path pays nothing; the count is only non-zero for the few milliseconds a dealloc is in flight). If teardown doesn't settle within 5s, start fails through the existing start-failure notification instead of entering the HAL against the wedged teardown. Together with the self-deferring prewarm, every path into configureSession/startEngine/prepareDirectAudioInputIfPossible is now behind the same fence.
Greptile SummaryThis PR fixes the launch-time freeze (#620) by ensuring a retired
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Measured proof of both the defect and the fix, since the reporter's spindump frames are truncated. Method. I built a probe dylib (loaded via Defect, on 1.6.4: the race precondition is present on essentially every idle route change — 37 bring-up events ( Fix, this branch, same workload: 0 bring-up events inside any foreign dealloc window across all 28 route changes; minimum Ordering argument (why it can't recur through this path): What this doesn't prove: the exact frames of the reporter's wedge (their spindump is truncated — full spindump requested on #620). What it does prove: 1.6.4 overlapped engine teardown with engine bring-up on every idle route change, that overlap is the only in-code path matching all three thread signatures in the spindump simultaneously, and this branch eliminates the overlap categorically under the same workload. |
…v#620) Review hardening of the teardown/bring-up sequencing: - A second route change arriving while a drain was still running could fire the rebuild completion synchronously (engineStorage already nil) and race the in-flight dealloc. Replace the per-retire completion with a drain fence: prewarmAudioEngineIfPossible now defers itself whenever any retired engine is still deallocating, re-checks when the fence fires, and repeat deferrals self-coalesce through the existing prepared guard. This also removes the bespoke generation counter. - Recording starts were not gated: start() now awaits pending drains (zero cost when none are in flight) and fails through the existing start-failure path if teardown does not settle within 5s, instead of building a new engine against it. - Track pendingEngineDrainCount on the main actor; it is incremented before a drain is scheduled and decremented only after dealloc has returned, so it can never read zero while a teardown is in flight. - Fix the drain-queue doc comment: one shared serial queue across all retired engines, not one per instance. Re-verified with the same probe workload (28 idle route changes): zero bring-up events inside any foreign dealloc window, minimum dealloc-exit to bring-up gap 21ms, app responsive throughout.
|
Proof re-validated after the review rework in 7679bb1 (the sequencing mechanism changed from per-retire completions to a drain fence, so the measurement was repeated): same probe, same workload — 28 idle route changes, 28 dealloc windows, 209 bring-up events. Result unchanged: 0 bring-up events inside any foreign dealloc window, 0 concurrent deallocs, minimum |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7679bb18ad
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if self.pendingEngineDrainCount > 0 { | ||
| guard await self.awaitPendingEngineDrains() else { | ||
| throw NSError( |
There was a problem hiding this comment.
This only fences drains that are already pending before startPreferredAudioCapture() runs. If the first engine.start() attempt fails, startEngine() still calls retireAudioEngine(reason: "start_retry") and immediately configureSession() for the retry without awaiting pendingEngineDrainCount, so device churn that triggers a start retry can still instantiate/prepare a new AVAudioEngine while the retired one is deallocating—the same HAL overlap this patch is trying to eliminate. The retry path needs the same drain fence before rebuilding.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right again — that was the last unfenced rebuild site (I re-audited every retireAudioEngine/configureSession/prepareDirectAudioInputIfPossible call site to confirm). Fixed in 727361d, though with postponement rather than the fence: startEngine()'s retry loop is synchronous, so it can't await. The invariant is "no dealloc concurrent with bring-up", and either ordering satisfies it — the retry path now detaches the failed engine into a local holder (detachAudioEngineForRetirement) and schedules all held drains in a defer when startEngine() exits (success or throw). A stopped, tap-removed engine sitting referenced for the duration of the loop is inert; its teardown handshake only happens at dealloc, which now cannot start until every retry attempt has finished bringing an engine up. retireAudioEngine and the retry path share one implementation (detach + scheduleRetiredEngineDrain), so the drains still land on the shared serial queue and still count toward the fence that gates prewarm/start/recovery. First-attempt-succeeds path is unchanged. Probe workload re-verified after the change: zero bring-up events inside any foreign dealloc window.
…es (altic-dev#620) The start-retry loop retired a failed engine and immediately rebuilt, which scheduled the dealloc concurrently with the retry's configureSession()/prepare() — the same HAL overlap the drain fence exists to prevent. The loop is synchronous, so awaiting the fence is not an option there; postponing the drain gives the same guarantee: engines detached during retries are held in a local list and their drains are scheduled only when startEngine() exits (success or throw), so no dealloc can run while any retry attempt is bringing an engine up. Splits retireAudioEngine into detachAudioEngineForRetirement + scheduleRetiredEngineDrain to make both orderings share one implementation. No behavior change on the first-attempt-succeeds path. Re-verified with the probe workload: zero bring-up events inside any foreign dealloc window across 28 route changes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 727361dfba
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| defer { | ||
| for retired in enginesRetiredDuringRetry { | ||
| self.scheduleRetiredEngineDrain(retired) | ||
| } |
There was a problem hiding this comment.
Fresh evidence after the retry fix: startCompatibilityAudioCapture() still calls setupEngineTap() immediately after startEngine() returns, but this defer schedules all failed-attempt engines for dealloc as soon as startEngine() exits. When attempt 1 fails and attempt 2 succeeds, the old engine can dealloc on the drain queue while the new engine is still reading inputFormat/installing the tap on the main actor, so the retry path can still overlap HAL teardown with bring-up. Keep these references alive until the whole compatibility setup, including tap installation, has completed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct — the hold ended one step too early; setupEngineTap()'s inputFormat/installTap are still bring-up. Fixed in the follow-up commit: startEngine(holdingRetiredEnginesIn:) now appends detached engines to a caller-owned list, and startCompatibilityAudioCapture() owns the defer, so drains are scheduled only after configure + start + tap install have all completed (or thrown — the defer covers both exits, and a subsequent failure-path retire drains behind the held ones on the same serial queue). Probe workload re-verified after the change: zero bring-up events inside any foreign dealloc window.
…ic-dev#620) startEngine()'s exit came before setupEngineTap(), so on a retry-then-success the failed engine's dealloc could begin while the new engine was still reading inputFormat and installing its tap. Ownership of the held references moves up to startCompatibilityAudioCapture(), whose defer schedules the drains only after the entire bring-up — configure, start, tap — has finished or thrown. startEngine() appends detached engines to a caller-owned list instead of draining on its own exit. Probe workload re-verified: zero bring-up events inside any foreign dealloc window across 28 route changes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9157cf5421
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if self.pendingEngineDrainCount > 0 { | ||
| guard await self.awaitPendingEngineDrains() else { |
There was a problem hiding this comment.
When pendingEngineDrainCount > 0, this new await suspends start() before isRunning is set; during that window stop paths such as GlobalHotkeyManager.stopRecordingIfNeeded still return early on !asrService.isRunning. In press-and-hold mode, or if the user toggles off while an idle route-change drain is pending, that stop request is dropped and start() continues into startPreferredAudioCapture() after the drain, leaving recording active after the user released/stopped.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid — the fence added the only suspension ahead of isRunning = true, and every hotkey stop path funnels through stopRecordingIfNeeded(), whose isRunning guard dropped a release landing in that window (normally milliseconds, but up to the 5 s drain bound in exactly the wedged-teardown case this branch is about). Fixed in 2eb0823: the funnel's early-return now calls abortStartIfPending(), which arms only while isStarting is set with isRunning still false — a stray key-up can't poison a later session — and start() re-checks the flag immediately after the fence, unwinding with the failure path's pipeline cleanup (no retire, no error notification; the flag clears in the existing isStarting defer on every exit).
Scope note: toggle mode's off-press during the window routes to start() and is absorbed by the isStarting re-entrancy guard (self-heals on the next press); treating a duplicate start() as a stop would break key-repeat during press-and-hold, so that stays as-is. The pre-existing sub-millisecond window (release processed before start()'s first synchronous slice) is unchanged — it exists on main today and would need a hotkey-side pending-start handshake if ever worth closing.
…ltic-dev#620) The drain fence gave start() its only suspension point ahead of isRunning = true, and every hotkey stop path funnels through stopRecordingIfNeeded(), whose isRunning guard silently dropped a release landing in that window — normally milliseconds, but up to the 5 s drain bound in exactly the wedged-teardown case this branch addresses. Capture would then come up after the key was already up and stay recording. stopRecordingIfNeeded()'s early-return now marks the in-flight start aborted via abortStartIfPending(), which arms only while isStarting is set with isRunning still false, so a stray key-up cannot poison a later session. start() re-checks the flag right after the fence and unwinds with the failure path's pipeline cleanup — no engine retire, no error notification — and the flag clears in the existing isStarting defer on every exit. Toggle mode's off-press during the window still routes to start() and is absorbed by the isStarting re-entrancy guard (self-heals on the next press); treating a duplicate start() as a stop would break key-repeat during press-and-hold, so it stays out of scope.
1 participant
Description
Fixes the permanent launch-time freeze reported in #620 (1.6.4 build 15).
What the spindump shows. The main thread is parked in
kevent_id— a dispatch workloop sync-wait — and the kernel's turnstile marks it as blocked on an in-process worker (0xcd437a) that is itself parked forever inpsynch_cvwait. A second worker (0xcd438f) is also stuck in a condition wait, and every CoreAudio/AudioToolbox thread (caulk messengers,AUScheduledParameterRefresher) went idle at the same instant, ~15 s after launch. That is a main-thread-waits-on-audio-workloop deadlock, the same family as #542.Root cause. Since the #542 fix, a retired
AVAudioEngine's final release always drains on a background queue — but the idle route-change path immediately builds the replacement engine on the main thread in the same call:-[AVAudioEngine dealloc]andinputNode/prepare()on a fresh engine both serialize inside the HAL. When they interleave badly, the main thread's bring-up parks behind the wedged teardown — permanently. Route changes fire in bursts right after launch (Bluetooth devices reconnecting, default device settling), which is why the freeze presents as "opens … freezes" and survives a reinstall: it needs only a route change while the app is idle with a prewarmed engine. The mid-recording recovery path (recoverAudioRoute) has the same overlap.The fix — never overlap engine teardown with engine bring-up:
scheduleRelease(onDrained:)lets callers order follow-up work strictly afterdeallochas completed.isRunningis re-checked after the suspension since recording can legitimately stop while the drain runs.Verified live
With the app idle and the engine prewarmed, programmatically flipping the macOS default output device (aggregate-device wrap → restore) triggers the exact code path. Before, retire and rebuild ran in the same instant; now the log shows the rebuild strictly after the drain, and the app stays fully responsive across repeated flips:
Type of Change
Related Issue or Discussion
Closes #620
Testing
swiftlint --strict --config .swiftlint.yml Sources— 0 violationsswiftformat --config .swiftformat Sourcesxcodebuild testsuite with CI's invocation — all suites passScreenshots / Video
Notes