fix(win): subprocess spawns work under bun run dev (--reload) on Windows (#122) - #175
Conversation
…ndows (#122) issue #122 'Extract: Unknown Error' on Windows: 'bun run dev' fails, running backend+frontend separately works. Root cause: dev:api launches uvicorn with --reload, so use_subprocess=True, and uvicorn 0.42's asyncio_loop_factory EXPLICITLY forces the SelectorEventLoop on Windows in that case (passed as loop_factory to asyncio_run, overriding any policy). The SelectorEventLoop has no subprocess support -> asyncio.create_subprocess_exec raises NotImplementedError. 'python backend/main.py' (no reload) uses ProactorEventLoop -> works. So an event-loop-policy fix is futile; the thread fallback is the fix. The ffmpeg extract path already routed through _spawn_async's thread fallback (landed in #157), but several other spawn sites used raw create_subprocess_exec and stayed broken on the dev loop: - add public spawn_subprocess() (drop-in for create_subprocess_exec) that routes through _spawn_with_retry -> _spawn_async (NotImplementedError -> thread fallback + EAGAIN retry); native asyncio path unchanged on supported loops. - fix _spawn_thread_fallback to forward cwd/env/etc. to subprocess.Popen (was silently dropping them -- breaks sonitranslate's cwd= pip install). - convert raw spawns: dub_generate atempo, tools ffprobe, gallery yt-dlp (x2), sonitranslate install (x4). translation_engines already had its own fallback. - tests: NotImplementedError -> thread fallback; cwd forwarding; stdin input (atempo); native path unchanged. No behavior change off the broken loop (macOS/Linux/Windows-prod): the native asyncio subprocess is still used; the fallback only triggers on NotImplementedError. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR introduces ChangesSubprocess abstraction and fallback
Sequence DiagramsequenceDiagram
participant API as API Endpoint
participant spawn as spawn_subprocess
participant retry as _spawn_with_retry
participant native as asyncio
participant fallback as Thread Fallback
API->>spawn: spawn_subprocess(cmd, ...)
spawn->>retry: delegate with args/kwargs
retry->>native: Try create_subprocess_exec
alt Event Loop Supports Subprocess
native-->>retry: Return native process
retry-->>spawn: Success path
spawn-->>API: Process object
else NotImplementedError
native-->>retry: Raise NotImplementedError
retry->>fallback: Launch subprocess.Popen in thread
fallback-->>retry: Return wrapped process
retry-->>spawn: Success via fallback
spawn-->>API: Process object (fallback)
end
API->>API: communicate() with process (identical API)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| from services import director, speech_rate, incremental | ||
| from services.ffmpeg_utils import find_ffmpeg, find_ffprobe | ||
| from services.ffmpeg_utils import find_ffmpeg, find_ffprobe, spawn_subprocess |
|
| Filename | Overview |
|---|---|
| backend/services/ffmpeg_utils.py | Core of the fix: adds spawn_subprocess public wrapper and fixes _spawn_thread_fallback to forward cwd/env/etc via **kwargs; _AsyncCompatProc.wait() does not update self.returncode after the process exits. |
| backend/api/routers/dub_generate.py | Replaces asyncio.create_subprocess_exec with spawn_subprocess for the atempo ffmpeg call; uses communicate(input=...) correctly so the thread fallback path handles stdin bytes without issues. |
| backend/api/routers/gallery.py | Two yt-dlp spawns converted to spawn_subprocess; both call communicate() before checking returncode, so the fallback path works correctly. |
| backend/api/routers/tools.py | ffprobe spawn converted to spawn_subprocess; straightforward change with no new logic. |
| backend/services/sonitranslate.py | Four subprocess spawns converted to spawn_subprocess; cwd=str(SONI_DIR) for the pip installs is now correctly forwarded through the fallback, fixing the previously silent drop. |
| tests/test_subprocess_fallback.py | Four focused regression tests covering: NotImplementedError fallback, cwd forwarding, stdin input= piping, and native-path preservation; good coverage of the changed behaviour. |
Sequence Diagram
sequenceDiagram
participant Caller as Router / Service
participant SW as spawn_subprocess()
participant SR as _spawn_with_retry()
participant SA as _spawn_async()
participant AIO as asyncio.create_subprocess_exec
participant FB as _spawn_thread_fallback()
participant PO as subprocess.Popen (thread)
Caller->>SW: "spawn_subprocess(*args, **kwargs)"
SW->>SR: "_spawn_with_retry(list(args), **kwargs)"
SR->>SA: "_spawn_async(cmd, **kwargs)"
SA->>AIO: "create_subprocess_exec(*cmd, **kwargs)"
alt Supported loop (Proactor / posix)
AIO-->>SA: asyncio.Process
SA-->>SR: Process
SR-->>SW: Process
SW-->>Caller: Process
else NotImplementedError (Windows SelectorEventLoop)
AIO-->>SA: NotImplementedError
SA->>FB: "_spawn_thread_fallback(cmd, **kwargs)"
Note over FB: pops stdout/stderr/stdin,<br/>forwards cwd/env/**kwargs to Popen
FB->>PO: "Popen(cmd, ..., cwd=..., **kwargs)"
PO-->>FB: Popen instance
FB-->>SA: _AsyncCompatProc(popen)
SA-->>SR: _AsyncCompatProc
SR-->>SW: _AsyncCompatProc
SW-->>Caller: _AsyncCompatProc
end
Caller->>Caller: "await proc.communicate([input=...])"
Comments Outside Diff (1)
-
backend/services/ffmpeg_utils.py, line 158-159 (link)wait()doesn't refreshself.returncodePopen.wait()blocks until the child exits and setsself._popen.returncode, but_AsyncCompatProc.wait()never copies that value back toself.returncode. Any caller that doesawait proc.wait()and then readsproc.returncodewill seeNone. Therun_ffmpegcleanup path (kill → wait →finally: if proc.returncode is None) hits exactly this: after kill+wait the returncode is stillNone, so the finally block repeats kill/wait unnecessarily. The new callsites in this PR always usecommunicate()(which does update returncode), so there's no visible breakage today, but the stale-returncode contract leaks the bug to any future caller that useswait()directly.
Reviews (1): Last reviewed commit: "fix(win): subprocess spawns work under `..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| **kwargs, # forward cwd / env / etc. so the fallback matches the async call | ||
| ) |
There was a problem hiding this comment.
Popen
asyncio.create_subprocess_exec accepts a limit kwarg (StreamReader buffer size) that subprocess.Popen does not. If any caller passes limit=… to spawn_subprocess, the **kwargs forwarding here will produce TypeError: __init__() got an unexpected keyword argument 'limit' on the fallback path only, making the failure mode hard to diagnose. None of the current callsites pass limit, but since spawn_subprocess is documented as a "drop-in replacement for asyncio.create_subprocess_exec", future callers are likely to try it. Filtering or documenting the exclusion would prevent a confusing failure.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/services/sonitranslate.py (1)
104-104:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftFix SoniTranslate venv layout for Windows (
bin/→Scripts/)
backend/services/sonitranslate.pyhardcodesSONI_VENV / "bin" / "pip"inis_venv_ready()andinstall(), andSONI_VENV / "bin" / "python"instart(). On Windows,python -m venvplaces executables underScripts\\, so venv readiness/pip install/start won’t use the correct binaries.- Apply the same cross-platform venv layout approach already used by
backend/engines/indextts/bootstrap.py(it branches toScripts/python.exeon win32).🛠️ Suggested platform-aware resolution
# module-level helper _VENV_BIN = "Scripts" if os.name == "nt" else "bin" _PIP_NAME = "pip.exe" if os.name == "nt" else "pip" _PY_NAME = "python.exe" if os.name == "nt" else "python"- pip = str(SONI_VENV / "bin" / "pip") + pip = str(SONI_VENV / _VENV_BIN / _PIP_NAME)Update
is_venv_ready()(Line 40) andstart()(Line 141) to use_VENV_BIN / _PIP_NAMEand_VENV_BIN / _PY_NAMErespectively.🤖 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/services/sonitranslate.py` at line 104, The venv paths are hardcoded to "bin" and non-Windows executable names; update backend/services/sonitranslate.py to use a platform-aware layout: add module-level helpers (_VENV_BIN = "Scripts" if os.name == "nt" else "bin", _PIP_NAME = "pip.exe" if os.name == "nt" else "pip", _PY_NAME = "python.exe" if os.name == "nt" else "python") and replace usages of SONI_VENV / "bin" / "pip" in is_venv_ready() and install() with SONI_VENV / _VENV_BIN / _PIP_NAME, and replace SONI_VENV / "bin" / "python" in start() with SONI_VENV / _VENV_BIN / _PY_NAME so the code selects Scripts\\ on Windows and bin on POSIX.
🧹 Nitpick comments (1)
tests/test_subprocess_fallback.py (1)
74-87: 💤 Low valueThis test doesn't verify the native path is actually taken.
The comment states the native
asynciopath is "used unchanged", but the assertions only check that output is produced. Since nothing forces a failure, this test would still pass even ifspawn_subprocessalways fell through to the thread fallback — so it doesn't guard against a regression that disables the native path on supported loops. Consider spying oncreate_subprocess_execto assert it was invoked.♻️ Assert native path was exercised
-def test_spawn_subprocess_native_path_when_loop_supports_it(): +def test_spawn_subprocess_native_path_when_loop_supports_it(monkeypatch): # On a loop WITH subprocess support (posix / Windows Proactor) the native # asyncio path is used unchanged — no behavior change off the broken loop. + real = ffmpeg_utils.asyncio.create_subprocess_exec + calls = [] + + async def _spy(*a, **k): + calls.append(a) + return await real(*a, **k) + + monkeypatch.setattr(ffmpeg_utils.asyncio, "create_subprocess_exec", _spy) + async def run(): proc = await ffmpeg_utils.spawn_subprocess( sys.executable, "-c", "print('hi')", stdout=asyncio.subprocess.PIPE, ) out, _ = await proc.communicate() return proc.returncode, out rc, out = asyncio.run(run()) assert rc == 0 assert b"hi" in out + assert calls, "native create_subprocess_exec was not used"🤖 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 `@tests/test_subprocess_fallback.py` around lines 74 - 87, The test test_spawn_subprocess_native_path_when_loop_supports_it currently only checks output and can pass even if ffmpeg_utils.spawn_subprocess used the thread fallback; update the test to spy/mock asyncio.create_subprocess_exec and assert it was called (or wrapped via a patch) when running the async run() so you verify the native asyncio path is exercised; locate references to ffmpeg_utils.spawn_subprocess and the test coroutine and add a mock/spy around asyncio.create_subprocess_exec (or the loop's create_subprocess_exec) to assert it was invoked during the test.
🤖 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.
Outside diff comments:
In `@backend/services/sonitranslate.py`:
- Line 104: The venv paths are hardcoded to "bin" and non-Windows executable
names; update backend/services/sonitranslate.py to use a platform-aware layout:
add module-level helpers (_VENV_BIN = "Scripts" if os.name == "nt" else "bin",
_PIP_NAME = "pip.exe" if os.name == "nt" else "pip", _PY_NAME = "python.exe" if
os.name == "nt" else "python") and replace usages of SONI_VENV / "bin" / "pip"
in is_venv_ready() and install() with SONI_VENV / _VENV_BIN / _PIP_NAME, and
replace SONI_VENV / "bin" / "python" in start() with SONI_VENV / _VENV_BIN /
_PY_NAME so the code selects Scripts\\ on Windows and bin on POSIX.
---
Nitpick comments:
In `@tests/test_subprocess_fallback.py`:
- Around line 74-87: The test
test_spawn_subprocess_native_path_when_loop_supports_it currently only checks
output and can pass even if ffmpeg_utils.spawn_subprocess used the thread
fallback; update the test to spy/mock asyncio.create_subprocess_exec and assert
it was called (or wrapped via a patch) when running the async run() so you
verify the native asyncio path is exercised; locate references to
ffmpeg_utils.spawn_subprocess and the test coroutine and add a mock/spy around
asyncio.create_subprocess_exec (or the loop's create_subprocess_exec) to assert
it was invoked during the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d5ba86a7-b0df-4f5c-9211-12c504c247c4
📒 Files selected for processing (6)
backend/api/routers/dub_generate.pybackend/api/routers/gallery.pybackend/api/routers/tools.pybackend/services/ffmpeg_utils.pybackend/services/sonitranslate.pytests/test_subprocess_fallback.py
…181) Completes the pro-output story: an Export-format selector (WAV / MP3) on the Stories toolbar. WAV stays fully client-side; MP3 routes the client-stitched WAV through a new backend POST /stories/encode (ffmpeg via the Windows-reload- safe spawn_subprocess, #175), with a strict format whitelist (mp3/m4b/ogg) and bitrate validation so the uploaded format can't inject ffmpeg args. Both the audiobook and per-character stems honor the selected format; MP3 falls back to WAV with a toast if ffmpeg is unavailable. - backend/api/routers/stories.py + main.py registration; temp-file cleanup. - frontend/src/api/stories.ts encodeAudio() (apiFetch: same-origin + PIN). - tests: format-whitelist 400, ffmpeg-missing 501, real mp3 encode (skips if ffmpeg absent). Backend 26 incl. router smoke; frontend 152/152, typecheck/ build/CJK ✓. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 participants
Closes the residual of #122 "Extract: Unknown Error" (Windows).
Root cause (definitive)
The reporter found that
bun run devfails but running backend + frontend separately works. Why:dev:apilaunchesuvicorn … --reload→use_subprocess=True.asyncio_loop_factoryexplicitly forces theSelectorEventLoopon Windows whenuse_subprocess=True(and passes it asloop_factorytoasyncio_run, so it overrides any event-loop policy).SelectorEventLoophas no subprocess support →asyncio.create_subprocess_execraisesNotImplementedError.python backend/main.py(no reload) uses theProactorEventLoop→ works.So a policy-based fix is futile (uvicorn overrides it). The thread-based fallback is the only robust fix.
Fix
The ffmpeg extract path already routed through
_spawn_async's thread fallback (#157), but other spawn sites used rawcreate_subprocess_execand stayed broken on the dev loop:spawn_subprocess()— public drop-in forcreate_subprocess_execthat routes through_spawn_with_retry → _spawn_async(NotImplementedError → thread fallback, plus EAGAIN retry). Native asyncio path unchanged on supported loops._spawn_thread_fallbacknow forwardscwd/env/etc. tosubprocess.Popen(it was silently dropping them — breaks sonitranslate'scwd=pip install in the fallback).translation_enginesalready had its own fallback.cwdforwarding; stdininput=(atempo); native path unchanged.Verify
tests/test_subprocess_fallback.py(4) ✓; router smoke (23) ✓.python main.py): the native asyncio subprocess is still used; the fallback only triggers onNotImplementedError.Cross-platform parity preserved; no engine/model state touched; local-first.
🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
Tests