From 56d0da0a519be10cb34d980d6fad1d5324974238 Mon Sep 17 00:00:00 2001 From: Rocky <72559939+hkgood@users.noreply.github.com> Date: Thu, 18 Jun 2026 21:00:18 +0800 Subject: [PATCH] fix: preview disc stuck at .processing after stop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous code path for the keyboard preview's ASR controller cancelled the consumer task at the exact moment it closed the audio stream: asrTask?.cancel() // ← kills the .final consumer asrTask = nil ... bufferContinuation?.finish() // tells ASR "no more audio" The cancellation cascaded: the for-await on the events stream exited → the AsyncStream's `continuation.onTermination` fired → ASR.cancel() ran → producer task was marked cancelled → the producer's `if !Task.isCancelled { yield(.final) }` guard suppressed the .final event. Net result: nobody told the UI to leave `.processing`, and the disc sat there forever. Fix (4 changes): 1) `stop()` no longer cancels the consumer. The consumer task exits naturally when the events stream finishes, sees the `.final` event the producer still yields, and transitions the phase out of `.processing`. This is the primary fix. 2) `start()` cancels any leftover `asrTask` at the entry point as a safety net — covers the "user smashes the disc twice quickly" race where a previous consumer is still draining. 3) `stop()` schedules a 3-second safety-net Task: if the ASR pipeline never produces a `.final` (analyzer hang, system glitch), force the phase back to `.idle` so the user isn't stuck. Normal recordings complete well under 3 seconds, so the timeout is only hit on the unhappy path. 4) `KeyboardPreviewSheet` adds `.onDisappear { asr.stop() }` so closing the sheet mid-recording releases the AVAudioSession and mic. `stop()` is idempotent (no-op on non-recording phases), safe to call here. State machine: `phase = .processing` now has TWO transition paths out — the consumer receiving `.final` (fast path) and the 3-second safety net (fallback). Both are required; the fast path is the common case, the fallback is the "guaranteed-progress" guarantee. Testability: `asrTask` was `private`; relaxed to `internal` so the regression test in `OSGKeyboardTests/PreviewASRControllerStateTests.swift` can install a known consumer task and assert `stop()` does not cancel it. The class is `@MainActor` so Swift 6 isolation rules still prevent production code outside the class from racing on it. Tests: - `testStopDoesNotCancelConsumerTask` — primary fix regression. - `testStopIsIdempotent` — `.onDisappear` after a manual stop doesn't misbehave. - 27/27 tests pass (25 existing + 2 new). - BUILD SUCCEEDED. 🤖 Generated with Claude Code --- OSGKeyboard/Views/KeyboardPreviewSheet.swift | 10 +++ OSGKeyboard/Views/PreviewASRController.swift | 48 ++++++++++++- .../PreviewASRControllerStateTests.swift | 70 +++++++++++++++++++ 3 files changed, 125 insertions(+), 3 deletions(-) create mode 100644 OSGKeyboardTests/PreviewASRControllerStateTests.swift diff --git a/OSGKeyboard/Views/KeyboardPreviewSheet.swift b/OSGKeyboard/Views/KeyboardPreviewSheet.swift index 24086b9..933bed0 100644 --- a/OSGKeyboard/Views/KeyboardPreviewSheet.swift +++ b/OSGKeyboard/Views/KeyboardPreviewSheet.swift @@ -101,6 +101,16 @@ struct KeyboardPreviewSheet: View { insertRecognizedText(new) asr.reset() } + // Tear down the ASR pipeline when the sheet leaves the screen + // (preview dismissed, app backgrounded mid-recording, etc.). + // Without this, a leftover `asrTask` keeps the AVAudioSession + // active and the mic permission in use after the user has + // moved on. `asr.stop()` is idempotent — it no-ops on + // `.idle`/`.denied`/`.error` — so it's safe to call here + // even when the disc is not currently recording. + .onDisappear { + asr.stop() + } } /// Real `TextField` (was a static placeholder HStack before the diff --git a/OSGKeyboard/Views/PreviewASRController.swift b/OSGKeyboard/Views/PreviewASRController.swift index cd1bc74..a237ed3 100644 --- a/OSGKeyboard/Views/PreviewASRController.swift +++ b/OSGKeyboard/Views/PreviewASRController.swift @@ -50,7 +50,13 @@ final class PreviewASRController: ObservableObject { private let asr: ASRService = ASRServiceFactory.make() private let audioEngine = AVAudioEngine() - private var asrTask: Task? + /// `internal` (not `private`) so the regression test in + /// `OSGKeyboardTests/PreviewASRControllerStateTests.swift` can + /// install a known consumer task and assert `stop()` doesn't + /// cancel it. The class is `@MainActor`-isolated, so the + /// natural Swift 6 isolation rules still prevent production + /// code outside the class from racing on it. + var asrTask: Task? private var bufferContinuation: AsyncStream.Continuation? private var didConfigureAudioSession = false private var didInstallTap = false @@ -65,6 +71,15 @@ final class PreviewASRController: ObservableObject { default: break } + // Cancel any leftover consumer task from a previous recording. + // Normally `stop()` lets the task run to completion (so it can + // see the `.final` and transition out of `.processing`), but if + // the user smashed the disc twice — stop, then immediately + // start — the previous task might still be draining. Cancel it + // here so we don't have two consumer tasks fighting over the + // same `events` stream. + asrTask?.cancel() + asrTask = nil phase = .requestingPermission currentPartial = "" lastFinal = "" @@ -111,8 +126,20 @@ final class PreviewASRController: ObservableObject { } func stop() { - asrTask?.cancel() - asrTask = nil + // Don't `asrTask?.cancel()` here — see the comment in + // `startEngineAndASR` for the full rationale. Short version: + // cancelling the consumer task at the same moment we close the + // audio stream also triggers the producer's + // `continuation.onTermination → self?.cancel()` cascade, which + // marks the producer's outer task as cancelled and skips the + // `.final` event. The UI is then left in `.processing` forever + // because no one schedules the transition out. The consumer + // task naturally exits when `events` finishes, so the right + // thing is to let it run. + // + // If a previous `asrTask` is somehow still running (e.g. the + // user smashed the disc twice quickly), `start()` cancels it + // at the entry point as a safety net. if didInstallTap { audioEngine.inputNode.removeTap(onBus: 0) didInstallTap = false @@ -128,6 +155,21 @@ final class PreviewASRController: ObservableObject { // Deactivate so the user's music resumes if the preview is // dismissed mid-recording. try? AVAudioSession.sharedInstance().setActive(false, options: .notifyOthersOnDeactivation) + + // Safety net: if the ASR pipeline never produces a `.final` + // (analyzer hang, system glitch, dropped continuation), force + // the UI back to idle after a short delay so the user isn't + // stuck. Normal recordings complete well under a second, so + // the 3-second budget is only hit on the unhappy path; if the + // pipeline finishes first and flips the phase to `.idle` (or + // `.error`), the check below no-ops. + Task { @MainActor [weak self] in + try? await Task.sleep(for: .seconds(3)) + guard let self else { return } + if self.phase == .processing { + self.phase = .idle + } + } } func reset() { diff --git a/OSGKeyboardTests/PreviewASRControllerStateTests.swift b/OSGKeyboardTests/PreviewASRControllerStateTests.swift new file mode 100644 index 0000000..d62ade2 --- /dev/null +++ b/OSGKeyboardTests/PreviewASRControllerStateTests.swift @@ -0,0 +1,70 @@ +// PreviewASRControllerStateTests.swift +// OSGKeyboard · Tests +// +// Locks in the state-machine contract for the keyboard preview's +// ASR controller. The previous bug was: `stop()` called +// `asrTask?.cancel()` on the consumer task that was waiting for the +// ASR `.final` event. The cancellation cascaded through +// `continuation.onTermination → self.cancel()` and suppressed the +// `.final` yield, leaving the controller in `.processing` forever +// because no one scheduled the transition out. The fix is: don't +// cancel on stop, let the consumer naturally see the `.final` and +// flip the phase. +// +// These tests don't drive a real ASR pipeline — they verify the +// state-machine contract directly. The class is `@MainActor` +// isolated, so all assertions happen on main. `asrTask` is +// `internal` (not `private`) precisely so this file can install a +// known consumer task and observe whether `stop()` cancels it. +// +// Note on the 3-second processing-timeout safety net: that +// fallback (a `Task { sleep 3s; if .processing → .idle }` block +// inside `stop()`) is not unit-tested here — testing it would +// require either a 3-second test or extracting the policy to a +// testable helper. The primary fix above removes the original +// bug outright; the timeout is a defensive safety net for a +// separate class of failure (analyzer hang) and is straightforward +// enough to read that a test adds little. If a future change +// touches the safety-net block, re-introduce a test that mocks +// the ASR to never yield `.final` and asserts the timeout fires. + +import XCTest +@testable import OSGKeyboard +import AVFoundation + +@MainActor +final class PreviewASRControllerStateTests: XCTestCase { + + /// Core fix: `stop()` MUST NOT cancel `asrTask`. The consumer + /// task is the only place the `.final` event can land, and + /// cancelling it leaves the UI in `.processing` forever. This + /// is the regression test for the original bug — if it ever + /// fails, the disc is stuck again. + func testStopDoesNotCancelConsumerTask() { + let controller = PreviewASRController() + let consumerTask = Task {} + controller.asrTask = consumerTask + + controller.stop() + + XCTAssertFalse( + consumerTask.isCancelled, + "stop() must not cancel the consumer task; the .final event needs the consumer alive to land" + ) + } + + /// Idempotency: calling `stop()` twice (e.g. `.onDisappear` + /// fires after the user already stopped manually) must not + /// misbehave — no double-cancel, no extra state transitions. + /// This is the contract `.onDisappear` relies on. + func testStopIsIdempotent() { + let controller = PreviewASRController() + let consumerTask = Task {} + controller.asrTask = consumerTask + + controller.stop() + controller.stop() + + XCTAssertFalse(consumerTask.isCancelled) + } +}