fix(engines): path-aware GPU-pool slot in SubprocessASRBackend.transcribe() (ASR sibling of #1298) - #1304
Merged
debpalash merged 4 commits intoJul 29, 2026
Conversation
…ribe() transcribe() had the same on-pool self-deadlock that generate() had (fixed in debpalash#1298): a bare no-op submitted to the GPU pool, but run_transcribe_guarded dispatches it via run_in_executor(_gpu_pool), already on a pool worker, so on a 1-worker (MPS) pool the no-op queued behind the job running it and timed out before the sidecar spawned. IsolatedFasterWhisperBackend on MPS hit this on every transcription. Mirror generate()'s path-aware slot block (on-pool skip via running_on_gpu_pool; off-pool _occupy hold) in transcribe(). The pattern is duplicated rather than extracted into a shared helper to avoid reworking generate(), which just shipped (debpalash#1298) with a CodeQL fix; extracting a shared contextmanager is a clean follow-up. Regression test added (transcribe dispatched on a pool worker).
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| that queued behind the very job running it and result(timeout=10) raised before | ||
| the sidecar spawned. This test reproduces that dispatch shape. | ||
| """ | ||
| import json |
| the sidecar spawned. This test reproduces that dispatch shape. | ||
| """ | ||
| import json | ||
| import struct |
| from pathlib import Path | ||
| from concurrent.futures import ThreadPoolExecutor | ||
|
|
||
| import pytest |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
|
| Filename | Overview |
|---|---|
| backend/services/subprocess_asr.py | The added import resolves the prior off-pool NameError, while the path-aware slot lifecycle releases its worker across success and exception paths. |
| backend/tests/test_subprocess_asr_slot_deadlock.py | Regression tests exercise the single-worker on-pool path and verify successful direct off-pool transcription followed by slot reuse. |
Reviews (3): Last reviewed commit: "test(engines): cover the off-pool transc..." | Re-trigger Greptile
Contributor
📝 WalkthroughWalkthrough
ChangesASR pool deadlock prevention
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 7 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (7 passed)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/services/subprocess_asr.py`:
- Around line 95-96: Add the missing module-scope threading import in
subprocess_asr.py before the reservation events are created in transcribe(),
ensuring off-pool calls can instantiate _held and _acquired without NameError.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5456fc4b-9eac-40fe-bc12-d90dfe6660dd
📒 Files selected for processing (2)
backend/services/subprocess_asr.pybackend/tests/test_subprocess_asr_slot_deadlock.py
transcribe()'s off-pool slot hold uses threading.Event(), but the module never imported threading — every subprocess-ASR transcribe raised NameError, and the three round-trip tests failed in CI. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Only the on-pool path had a test, so `threading.Event()` in the off-pool branch shipped with `threading` never imported — every direct caller hit NameError before the sidecar started. Both bots caught it on review; nothing in the suite did. A branch with no test is how a one-word bug reaches CI. Also asserts the slot is genuinely released afterwards. Fails without the import fix; the pre-existing on-pool test still passes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/tests/test_subprocess_asr_slot_deadlock.py`:
- Around line 141-152: Update the subprocess deadlock test around caller.start()
and caller.join() to assert not caller.is_alive() immediately after the timeout
before reading box. Ensure the caller thread is joined or otherwise confirmed
terminated before b.shutdown() and pool.shutdown() execute in the finally block.
- Around line 141-149: Update the test around the transcription caller and pool
marker task so the stub response is blocked while transcription is still in
progress. Submit the follow-up task before releasing the stub, assert it remains
pending during transcription, then release the stub and verify both
transcription and the marker complete successfully.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a75c2b82-7e93-47a6-a532-da6847ee04ac
📒 Files selected for processing (1)
backend/tests/test_subprocess_asr_slot_deadlock.py
Comment on lines
+141
to
+149
| try: | ||
| caller.start() | ||
| caller.join(timeout=30) | ||
| assert "error" not in box, f"off-pool transcribe failed: {box.get('error')}" | ||
| assert "segments" in box["result"] | ||
|
|
||
| # The slot must be back once the call returned. | ||
| marker = pool.submit(lambda: "ran") | ||
| assert marker.result(timeout=10) == "ran" |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Test the slot while transcription is still running.
The follow-up task is submitted only after caller.join() returns, so this test passes even if the off-pool path never holds a slot and cannot detect pool oversubscription. Block the stub response, submit the second pool task during transcription, assert it remains pending, then release the stub and verify completion.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/test_subprocess_asr_slot_deadlock.py` around lines 141 - 149,
Update the test around the transcription caller and pool marker task so the stub
response is blocked while transcription is still in progress. Submit the
follow-up task before releasing the stub, assert it remains pending during
transcription, then release the stub and verify both transcription and the
marker complete successfully.
Comment on lines
+141
to
+152
| try: | ||
| caller.start() | ||
| caller.join(timeout=30) | ||
| assert "error" not in box, f"off-pool transcribe failed: {box.get('error')}" | ||
| assert "segments" in box["result"] | ||
|
|
||
| # The slot must be back once the call returned. | ||
| marker = pool.submit(lambda: "ran") | ||
| assert marker.result(timeout=10) == "ran" | ||
| finally: | ||
| b.shutdown() | ||
| pool.shutdown(wait=False) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Fail cleanly when the caller times out.
The timeout from caller.join(timeout=30) is ignored; the test may access missing results and call b.shutdown() while the daemon thread is still using b. Assert not caller.is_alive() before inspecting box, and ensure the caller is joined before teardown.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/test_subprocess_asr_slot_deadlock.py` around lines 141 - 152,
Update the subprocess deadlock test around caller.start() and caller.join() to
assert not caller.is_alive() immediately after the timeout before reading box.
Ensure the caller thread is joined or otherwise confirmed terminated before
b.shutdown() and pool.shutdown() execute in the finally block.
debpalash
added a commit
that referenced
this pull request
Jul 29, 2026
Greptile P1: the crash marker records HOW the process died, not which subsystem was running — a segfault during transcription looks identical to one during synthesis. Naming only the TTS escape hatch sent ASR crashes to a fix that leaves the crashing path untouched. Both are now offered so the user picks the one they were using; #1304 supplies the ASR side. CodeRabbit Major: the new guidance was hardcoded English, which the localization rule forbids. All four hints in crashCauseHint now route through i18next with the English as defaultValue — so a missing key still renders exactly what it rendered before (no regression, no test churn) while the strings become translatable. crash_port_in_use, crash_oom_kill and crash_native_fault are translated in all 21 locales. Also fixes a test that claimed to prove repeat-fault behaviour while calling the hint once; it now asserts what the message actually has to contain. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
debpalash
added a commit
that referenced
this pull request
Jul 29, 2026
* fix(crash): stop blaming VRAM for native faults #1275 (Windows 0xC0000005 on an RTX 2080 SUPER) and #1293 (SIGSEGV on Linux) both fell through to "you ran out of VRAM while loading the ASR model" — so the advice was to flush a model that had nothing to do with it. A segfault is bad machine code, not slow memory exhaustion; the real causes are a GPU driver that disagrees with the bundled CUDA runtime, or a weight file that downloaded incompletely and is being memory-mapped. Windows has no signals here, so the shell sees the raw NTSTATUS as a negative exit code — those are matched explicitly or they read as an ordinary non-zero exit. Deliberately narrow: only SIGILL and SIGSEGV, whose numbers are identical on every POSIX platform. SIGABRT stays on the VRAM path because abort() is how a fatal CUDA error exits, including an async out-of-memory — an existing test pins that, and it caught this when the first cut was too greedy. SIGBUS is excluded because its number is platform-dependent (7 on Linux, 10 on macOS, where 10 is SIGUSR1 on Linux). Repeat offenders are now pointed at the crash-isolated subprocess engine that landed in #1292 — it takes the sidecar down instead of the whole backend. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(crash): offer both isolated engines, and translate the guidance Greptile P1: the crash marker records HOW the process died, not which subsystem was running — a segfault during transcription looks identical to one during synthesis. Naming only the TTS escape hatch sent ASR crashes to a fix that leaves the crashing path untouched. Both are now offered so the user picks the one they were using; #1304 supplies the ASR side. CodeRabbit Major: the new guidance was hardcoded English, which the localization rule forbids. All four hints in crashCauseHint now route through i18next with the English as defaultValue — so a missing key still renders exactly what it rendered before (no regression, no test churn) while the strings become translatable. crash_port_in_use, crash_oom_kill and crash_native_fault are translated in all 21 locales. Also fixes a test that claimed to prove repeat-fault behaviour while calling the hint once; it now asserts what the message actually has to contain. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This was referenced Jul 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1303.
transcribe()had the identical on-pool self-deadlock thatgenerate()had (fixed in #1298): a bare no-op submitted to the GPU pool, butrun_transcribe_guardeddispatches it viarun_in_executor(_gpu_pool), already on a pool worker, so on a 1-worker (MPS) pool the no-op queues behind the job running it and times out before the sidecar spawns. IsolatedFasterWhisperBackend on MPS hit this on every transcription.Changed
generate()'s path-aware slot block (on-pool skip viarunning_on_gpu_pool(); off-pool_occupyhold) intranscribe(). The pattern is duplicated rather than extracted into a shared helper to avoid reworkinggenerate(), which just shipped (fix(engines): path-aware GPU-pool slot in SubprocessBackend.generate() (on-pool skip + off-pool hold) #1298) with a CodeQL fix; extracting a shared_gpu_pool_slot()contextmanager is a clean follow-up.Tests
Updated
SubprocessASRBackend.transcribe()to avoid self-deadlock on one-worker GPU pools by skipping GPU-slot acquisition when already executing on a pool worker (running_on_gpu_pool()), and otherwise using an off-pool occupancy hold backed by athreading.Eventwith a 10s wait, cancellation on timeout, and correct slot/event release. Added a regression test that stubs the subprocess ASR engine and verifies (1) pool-workertranscribe()completes and (2) off-pool transcription holds the slot so a queued pool job doesn’t start until release. Main risk to review is the concurrency/timeout and slot-release correctness under edge scheduling (e.g., MPS/isolated subprocess backends).