fix(speech): honor task cancellation in recorder poll loop#204
fix(speech): honor task cancellation in recorder poll loop#204SebTardif wants to merge 3 commits into
Conversation
Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
|
Codex review: needs real behavior proof before merge. Reviewed June 29, 2026, 10:21 AM ET / 14:21 UTC. Summary Reproducibility: yes. for a source-level cancellation concern: current main has an unowned Whisper recorder poll task and a Review metrics: 2 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Keep the handle-and-guard direction, but wire recorder teardown into dismissal/deinit paths and add production-matched regression coverage or redacted live lifecycle proof before merge. Do we have a high-confidence way to reproduce the issue? Yes for a source-level cancellation concern: current main has an unowned Whisper recorder poll task and a Is this the best way to solve the issue? No, not yet: storing the task handle and checking cancellation is the right core direction, but the patch must also cover dismissal/teardown paths and prove the actual recognizer lifecycle. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against dda07c245fea. Label changesLabel changes:
Label justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
@clawsweeper[bot]
This PR is not superseded by #193. They fix the same bug class (
There is zero code overlap between the two PRs.
Fair point. The |
The Whisper recorder observer Task was fire-and-forget (handle discarded at line 279), so stopListening() could not cancel the poll loop. The Task.isCancelled guard was defensive-only with no lifecycle path that triggered it. Store the task handle in recorderObserverTask and cancel it in stopListening() before stopping the recorder, making the cancellation guard exercised through a real code path. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
Reproduces the try? Task.sleep poll-loop pattern from observeRecorderState() and demonstrates that the unfixed version ignores cancellation (runs all iterations) while the fixed version with Task.isCancelled guard exits after ~2 iterations. Usage: swiftc -parse-as-library -o /tmp/prove-speech scripts/prove-speech-poll-cancellation.swift && /tmp/prove-speech Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
clawsweeper
Bot
added
the
merge-risk: 🚨 availability
clawsweeper
Bot
added
rating: 🦪 silver shellfish
1 participant
Summary
The
observeRecorderState()poll loop inSpeech.swiftusestry? await Task.sleep(nanoseconds:)to throttle polling. Thetry?silently discardsCancellationError, causing the loop to run indefinitely when the parent task is cancelled.Additionally, the observer task handle was discarded (fire-and-forget
Task { }), sostopListening()had no way to cancel the poll loop. TheTask.isCancelledguard was defensive-only with no lifecycle path that triggered it.Problem
When the speech recognizer's parent task is cancelled (e.g., view dismissed or app entering background), the
observeRecorderStateloop continues polling the recorder'sisRecordingandtranscriptstate every 100ms. The loop only exits whenself.isListeningbecomes false orrecorder.isRecordingbecomes false, but neither is set by task cancellation.The
try? await Task.sleeppattern swallowsCancellationErroralong with other errors. Without an explicit cancellation check, the cooperative cancellation contract of Swift Structured Concurrency is broken.Fix
Two changes:
Store the observer task handle in
recorderObserverTask(was fire-and-forgetTask { }). Cancel it instopListening()before stopping the recorder. This connects the cancellation guard to a real lifecycle path.Add
guard !Task.isCancelled else { break }after thetry? await Task.sleepcall. This is the standard Swift pattern for honoring cooperative cancellation in poll loops that usetry?.Origin
The poll loop was introduced in commit
babc87d58(2025-07-30, Peter Steinberger) as part of the original speech recognizer implementation, approximately 11 months ago.Real behavior proof
Behavior addressed: The
observeRecorderState()poll loop inSpeech.swiftignores task cancellation becausetry? await Task.sleepsilently discardsCancellationError. The observer task handle was also discarded, sostopListening()could not cancel the loop. Dismissing the speech view or the app entering background leaves the loop running indefinitely.Real environment tested: macOS 26, Swift 6.3.3 (swiftlang-6.3.3.1.3), built from patched source on
fix/speech-poll-cancellationbranch.Exact steps or command run after this patch:
Evidence after fix: terminal output from the standalone cancellation proof:
Task handle storage and cancellation path verified:
Observed result after fix: The unfixed pattern runs all 20 iterations (ignoring cancellation) because
try?causesTask.sleepto return instantly on a cancelled task without throwing. The fixed pattern exits after 2 iterations becauseTask.isCancelledcatches the cancellation state. The stored task handle ensuresstopListening()can cancel the observer through a real lifecycle path.What was not tested: Live cancellation with active speech recognition (requires microphone access and SFSpeechRecognizer authorization). The standalone proof reproduces the exact poll-loop pattern from observeRecorderState().
Related
fix(capture): honor stop during watch transient backoffwaitForImage: commitsdd6d47fd,dc75fe4f