Skip to content

Phase 2 Plan 02-01: SubprocessBackend primitive (Wave 1 of Phase 2) - #97

Merged
debpalash merged 2 commits into
mainfrom
phase2-plan-02-01-subprocess-backend
May 20, 2026
Merged

Phase 2 Plan 02-01: SubprocessBackend primitive (Wave 1 of Phase 2)#97
debpalash merged 2 commits into
mainfrom
phase2-plan-02-01-subprocess-backend

Conversation

@debpalash

@debpalash debpalash commented May 20, 2026

Copy link
Copy Markdown
Owner

Summary

Lands the durable SubprocessBackend primitive — the architectural keystone Plans 02-03 (IndexTTS migration), Phase 3 (Supertonic-3), and Phase 4 (GGUF / Singing) plug into.

  • New base class backend/services/subprocess_backend.py (~370 LOC) — long-lived sidecar process via subprocess.Popen, length-prefixed JSON wire protocol, GPU-pool slot acquire/release, atexit teardown, stderr drain, op allowlist, 64 MB frame cap. No multiprocessing — only subprocess.Popen (Locked Decision D4 / Pitfall 1).
  • Echo sidecar backend/engines/_echo/main.py — permanent CI regression infrastructure. Stdlib-only (no torch), runs under the parent's sys.executable. DO NOT DELETE.
  • Engine-registry wrap in backend/services/tts_backend.pylist_backends() now wraps each is_available() in try/except so one broken engine can't blank the picker (ENGINE-05). Adds last_error and isolation_mode keys per entry for the Compat Matrix UI in Plan 02-04 (ENGINE-06).
  • 19 new tests (test_subprocess_backend.py + test_tts_backend_registry.py).

Requirements covered

  • ENGINE-01 (engine isolation primitive exists, tested, documented)
  • ENGINE-05 (one broken engine can't blank the picker — graceful degradation wrap in list_backends)
  • ENGINE-06 (foundation)last_error and isolation_mode keys delivered on the existing /engines response; Plan 02-04 UI consumes them.

Threat-model mitigations

ID Threat Mitigation
T-02-01 Length-prefix DoS (4 GB alloc) MAX_FRAME_BYTES = 64 * 1024 * 1024; _recv raises IOError("frame too large") before allocation
T-02-02 GPU slot leak on sidecar death pool.submit(lambda: None).result() slot held; try/finally surrounds send/recv so a sidecar exception always returns the slot
T-02-03 Token bytes in sidecar stderr Background drain thread routes stderr lines through the parent's root logger — Phase 1's HFTokenRedactor filter strips secrets
T-02-04 Compromised sidecar emitting unknown ops PARENT_INBOUND_OPS allowlist enforced in _recv; unknown ops logged + dropped
T-02-05 Tauri group-kill scope (process group escape) start_new_session=True on Unix; CREATE_NEW_PROCESS_GROUP on Windows

New artifact paths

  • backend/services/subprocess_backend.py (base class + protocol constants)
  • backend/engines/__init__.py, backend/engines/_echo/__init__.py, backend/engines/_echo/main.py (regression sidecar)
  • tests/backend/services/test_subprocess_backend.py (13 tests)
  • tests/backend/services/test_tts_backend_registry.py (6 tests)
  • .planning/phases/02-engine-isolation-subprocessbackend-indextts-wav-export-dubbi/02-01-SUMMARY.md (public-API summary for downstream plans)

Files modified

  • backend/services/tts_backend.py — adds _LAST_ERRORS cache + wraps list_backends() + adds the two new response keys. No other backend class touched (D1 — IndexTTS migration is Plan 02-03; SoniTranslate is locked off until v0.4).

Hard constraints honored

  • Existing v0.2.7 IndexTTS installs: not affected — this plan adds the primitive only; no engine migration here.
  • Cross-platform: every Popen flag, every test, runs unchanged on macOS / Linux / Windows. Tests use psutil (cross-platform) rather than os.kill(pid, 0) (Unix-only semantics).
  • HF_HOME / HF_TOKEN inheritance: env passes through os.environ.copy() — Phase 1 AUTH-04 token injection on the parent side flows to the child unchanged. Verified by test_env_forwarding_contract.
  • SoniTranslate untouched (D1 locked): git diff main backend/services/sonitranslate.py is empty.
  • Smoke test still passes: uv run pytest tests/smoke/ -q → 4 passed.

Test plan

  • uv run pytest tests/backend/services/test_subprocess_backend.py -v → 13 passed
  • uv run pytest tests/backend/services/test_tts_backend_registry.py -v → 6 passed
  • uv run pytest tests/smoke/ -q → 4 passed
  • uv run pytest tests/ -q --ignore=tests/manual → 367 passed, 10 skipped, 13 xfailed, 1 xpassed
  • Grep gates: no mp.Process / mp.fork / mp.spawn in subprocess_backend.py; atexit.register, os.environ.copy(), start_new_session/CREATE_NEW_PROCESS_GROUP, MAX_FRAME_BYTES = 64 * 1024 * 1024 all present
  • Linux + Windows CI matrix (delegated to CI on this PR)
  • Manual curl http://localhost:3900/engines shows last_error and isolation_mode keys on every entry (smoke verification — non-gating)

Deviations from plan (full detail in 02-01-SUMMARY.md)

  1. issubclass(cls, SubprocessBackend)getattr(cls, "_is_subprocess_isolated", False) because token-resolver test fixtures purge sys.modules["services"], producing a re-imported SubprocessBackend whose issubclass returns False for subclasses that closed over the original class. The duck-typed marker is robust under that pattern.
  2. _recv_with_timeout uses a threading.Timer watchdog rather than selectors so it works on Windows (which can't select on subprocess pipes).
  3. GPU-slot release is implicit (no-op submitted to _get_gpu_pool() returns its worker the instant it completes); the structural try/finally in generate() covers the failure path.

What this unblocks

  • Plan 02-03 (IndexTTS migration) — has the spawn primitive, the wire protocol, and the env-forwarding contract it needs.
  • Plan 02-04 (Compat Matrix UI) — has the last_error + isolation_mode data to render.
  • Phase 3 Wave 1 (Supertonic-3) — sidecar can speak the same wire protocol; nothing engine-specific in the base.

Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Subprocess-isolated TTS engine execution for improved stability and automatic crash recovery
    • Backend listings now display isolation mode (in-process or subprocess) for each engine
    • Enhanced error tracking with last error information for backend failures
  • Improvements

    • More robust error handling and subprocess resource management
    • Improved cleanup and recovery from engine crashes

Review Change Stack

debpalash and others added 2 commits May 20, 2026 06:45
…NE-05 wrap

Lands the durable SubprocessBackend primitive — the architectural keystone
that Plans 02-03 (IndexTTS migration), Phase 3 (Supertonic-3), and
Phase 4 (GGUF / Singing) plug into.

Files added:
  - backend/services/subprocess_backend.py — base class owning spawn,
    shutdown, _send/_recv (length-prefixed JSON), GPU-slot acquire-release,
    atexit teardown, stderr drain, op allowlist (T-02-04), and 64 MB
    frame cap (T-02-01). No multiprocessing — subprocess.Popen
    exclusively so subclasses can target a *different* venv's interpreter
    (Locked Decision D4 / Pitfall 1).
  - backend/engines/_echo/main.py — permanent CI regression sidecar.
    Stdlib-only, runs under the parent's sys.executable. Implements
    ready/ping-pong/synthesize/shutdown plus test-only probe_env and
    emit_unknown ops for env-forwarding and op-allowlist tests. DO NOT
    DELETE — the round-trip test depends on this file.
  - tests/backend/services/test_subprocess_backend.py — 13 tests:
    round-trip, health_check, no-zombie, shutdown idempotency, env
    forwarding (HF_TOKEN/HF_HOME/HF_ENDPOINT/HF_HUB_CACHE), oversize
    frame, short read, op-allowlist drop, op-allowlist constant shape,
    sidecar-crash recovery, no-multiprocessing grep gate, MAX_FRAME_BYTES.
  - tests/backend/services/test_tts_backend_registry.py — 6 tests for
    list_backends() resilience + shape + isolation_mode + last_error
    caching + existing-engines preservation + install_hint passthrough.

Files modified:
  - backend/services/tts_backend.py:
    * Adds module-level _LAST_ERRORS dict for ENGINE-06.
    * Rewrites list_backends() to wrap each is_available() in try/except
      so one broken engine cannot blank the picker (ENGINE-05).
    * Adds last_error + isolation_mode keys to each response entry
      (ENGINE-06 UI in Plan 02-04 consumes via the same /engines route).
    * Uses a duck-typed _is_subprocess_isolated marker rather than
      issubclass(cls, SubprocessBackend) because test fixtures (token
      resolver suite) purge sys.modules["services"] between tests and the
      re-imported SubprocessBackend would be a different class object.

Threat-model mitigations (Plan 02-01 frontmatter):
  T-02-01 DoS via length-prefix → MAX_FRAME_BYTES = 64 * 1024 * 1024
  T-02-02 GPU slot leak on sidecar death → try/finally in generate
  T-02-03 token bytes in stderr → drained via parent logger
          (HFTokenRedactor from Phase 1 already on root)
  T-02-04 unknown ops from compromised sidecar → PARENT_INBOUND_OPS
          allowlist, unknown frames logged and dropped
  T-02-05 Tauri group-kill scope → start_new_session=True on Unix /
          CREATE_NEW_PROCESS_GROUP on Windows

Verification:
  - 337 passed, 6 skipped, 12 xfailed, 1 xpassed (full suite,
    `uv run pytest tests/ --ignore=tests/manual`)
  - All 19 new tests pass on macOS Apple Silicon
  - Smoke tests still pass: `uv run pytest tests/smoke/ -q` → 4 passed
  - SoniTranslate untouched (D1 locked decision)
  - Zero new Python dependencies

Closes part of ENGINE-01 + ENGINE-05.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Documents the SubprocessBackend public API so Plan 02-03 (IndexTTS) and
Phase 3 (Supertonic-3) authors don't need to re-read the source.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

This PR lands subprocess-isolated TTS backend infrastructure: a new SubprocessBackend base class that spawns engines as isolated processes with length-prefixed JSON IPC, an echo sidecar for CI regression testing, registry enhancements to track errors and isolation mode, and comprehensive test coverage validating protocol safety, crash recovery, and environment forwarding.

Changes

Subprocess Isolation Architecture

Layer / File(s) Summary
Phase 02 Documentation & Package Structure
.planning/phases/.../02-01-SUMMARY.md, backend/engines/__init__.py, backend/engines/_echo/__init__.py
Phase summary documents the completed subprocess backend design, wire protocol, tested invariants, and follow-on requirements for IndexTTS/Supertonic-3. Module docstrings explain architecture, sidecar permanence, and system Python requirement.
SubprocessBackend Implementation
backend/services/subprocess_backend.py
Base class for subprocess isolation with lifecycle hooks (_spawn, idempotent shutdown), JSON framing with 4-byte big-endian length prefixes and 64 MiB size bounds, op allowlists, watchdog timeout handlers to kill hung sidecars, GPU slot coordination, and stderr draining with token redaction.
Echo Sidecar: Test Infrastructure
backend/engines/_echo/main.py
Stdlib-only CI regression sidecar implementing the length-prefixed JSON protocol: responds to ping with pong, synthesize with 1s int16 PCM silence base64-encoded at requested sample rate, shutdown with exit(0), and test-only probe_env/emit_unknown operations; includes optional crash simulation.
Registry: Error Tracking & Isolation Mode
backend/services/tts_backend.py
Extends list_backends() to catch and cache availability exceptions per backend, add last_error field (cleared after recovery), and detect isolation_mode via duck-typed subprocess marker.
SubprocessBackend Test Suite
tests/backend/services/test_subprocess_backend.py
End-to-end validation using echo sidecar: round-trip audio shape/dtype, health-check ping semantics, idempotent shutdown with zombie/PID checks, HF environment forwarding, frame-protocol DoS guards (oversized/short read rejection), inbound op allowlist enforcement, crash recovery without deadlock, and policy checks (no multiprocessing/fork, 64 MiB constant).
Registry Test Suite
tests/backend/services/test_tts_backend_registry.py
Validates registry resilience to is_available() failures with proper error caching/clearing, exact entry schema (all required/no extra keys), subprocess vs. in-process isolation mode differentiation, and preservation of existing engine IDs and install hints.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • debpalash/OmniVoice-Studio#71: Main PR adds the backend/main.py --health-check behavior and /health polling semantics that this PR's subprocess backends depend on for startup validation and availability monitoring.

Poem

🐰 Subprocesses spin in isolation,
JSON frames in conversation,
Echo sidecars test the way,
Crashes caught, no locks delay!
Onward to IndexTTS,
The rabbit builds with happiness! 🎶

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 47.22% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically identifies the main change as the SubprocessBackend primitive implementation for Phase 2, Plan 02-01.
Description check ✅ Passed The PR description is comprehensive, covering summary, changes, requirements, threat mitigations, test results, and constraints. It follows the template structure with all critical sections populated.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch phase2-plan-02-01-subprocess-backend

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 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_backend.py`:
- Around line 207-246: The shutdown() method modifies and reads self._proc
without acquiring the same lock used in health_check()/generate(), causing
races; wrap the entire shutdown() body (reads, _send call, terminate/kill
sequence and the final self._proc = None) with the instance lock (self._lock) to
serialize access to _proc, so that other methods (health_check, generate) cannot
mutate or observe _proc concurrently; ensure you hold self._lock while checking
if proc is None, calling self._send({"op": "shutdown"}),
waiting/terminating/killing the process, and setting self._proc = None.
- Around line 362-394: The _recv method currently tail-recurses by returning
self._recv() when a frame has an unknown op, which can overflow the call stack;
change this to an iterative loop: wrap the frame-read/parse/validate sequence
inside a while True loop that continues reading the next frame when op not in
PARENT_INBOUND_OPS (logging with logger as before), and only return msg when op
is allowed or return None/raise on EOF/errors; keep uses of _read_exact,
MAX_FRAME_BYTES, json.loads, and self._proc.stdout intact and preserve existing
error handling and logging.
- Around line 302-342: The current pattern submits a no-op via pool.submit and
.result() too early so the ThreadPoolExecutor slot is released before GPU work
starts; instead make the submitted task block for the entire generate lifecycle
and only complete in the finally block so the worker remains reserved while you
run self._spawn(), _send(), and _recv_with_timeout(). Concretely: replace the
immediate no-op+result() with submitting a blocking callable (e.g., one that
waits on a threading.Event or similar) and keep that Event and the resulting
slot_future live while holding self._lock and performing synthesize (the code
around slot_future, self._spawn, self._send, and self._recv_with_timeout); in
the finally block set the Event (or otherwise unblock the callable) so the
slot_future can finish and return the worker to the pool. Ensure
slot_future.cancel() semantics are adjusted accordingly.

In `@backend/services/tts_backend.py`:
- Around line 1217-1240: The code currently builds msg = f"{type(exc).__name__}:
{exc}" and persists/returns it; change this to keep the full msg for logging but
create a sanitized_msg (e.g., only type name and a generic note or
truncated/escaped exception text without paths/tokens) and use sanitized_msg
when writing to _LAST_ERRORS and the out dict fields "reason" and "last_error";
keep the original msg in logger.warning or logger.debug for diagnostics but
never persist or return it. Ensure you update the assignment sites where
_LAST_ERRORS[bId] and the out.append(...) use msg to use sanitized_msg instead.
🪄 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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 3fa96180-04b6-420c-abf4-7e92ff54977f

📥 Commits

Reviewing files that changed from the base of the PR and between c6e9bbc and fe4c04f.

📒 Files selected for processing (8)
  • .planning/phases/02-engine-isolation-subprocessbackend-indextts-wav-export-dubbi/02-01-SUMMARY.md
  • backend/engines/__init__.py
  • backend/engines/_echo/__init__.py
  • backend/engines/_echo/main.py
  • backend/services/subprocess_backend.py
  • backend/services/tts_backend.py
  • tests/backend/services/test_subprocess_backend.py
  • tests/backend/services/test_tts_backend_registry.py

Comment on lines +207 to +246
def shutdown(self) -> None:
"""Idempotent. Sends {op:shutdown}; falls back to terminate/kill."""
proc = self._proc
if proc is None:
return
try:
try:
# Best effort — sidecar may already be dead.
self._send({"op": "shutdown"})
except Exception:
pass
try:
proc.wait(timeout=3)
except subprocess.TimeoutExpired:
logger.warning(
"[%s] sidecar did not exit on shutdown frame; terminating",
self.id,
)
try:
proc.terminate()
except Exception:
pass
try:
proc.wait(timeout=2)
except subprocess.TimeoutExpired:
logger.warning(
"[%s] sidecar did not exit on SIGTERM; killing",
self.id,
)
try:
proc.kill()
except Exception:
pass
try:
proc.wait(timeout=2)
except Exception:
pass
finally:
self._proc = None

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Serialize shutdown() with self._lock to avoid _proc races.

Line 207 modifies _proc without the same lock used by health_check()/generate(). Concurrent shutdown can invalidate _proc mid send/recv and cause flaky failures.

💡 Suggested fix
 def shutdown(self) -> None:
     """Idempotent. Sends {op:shutdown}; falls back to terminate/kill."""
-    proc = self._proc
-    if proc is None:
-        return
-    try:
+    with self._lock:
+        proc = self._proc
+        if proc is None:
+            return
+        try:
             try:
                 # Best effort — sidecar may already be dead.
                 self._send({"op": "shutdown"})
             except Exception:
                 pass
@@
-    finally:
-        self._proc = None
+        finally:
+            self._proc = None
🧰 Tools
🪛 Ruff (0.15.13)

[error] 216-217: try-except-pass detected, consider logging the exception

(S110)


[warning] 216-216: Do not catch blind exception: Exception

(BLE001)


[error] 227-228: try-except-pass detected, consider logging the exception

(S110)


[warning] 227-227: Do not catch blind exception: Exception

(BLE001)


[error] 238-239: try-except-pass detected, consider logging the exception

(S110)


[warning] 238-238: Do not catch blind exception: Exception

(BLE001)


[error] 242-243: try-except-pass detected, consider logging the exception

(S110)


[warning] 242-242: Do not catch blind exception: Exception

(BLE001)

🤖 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/subprocess_backend.py` around lines 207 - 246, The
shutdown() method modifies and reads self._proc without acquiring the same lock
used in health_check()/generate(), causing races; wrap the entire shutdown()
body (reads, _send call, terminate/kill sequence and the final self._proc =
None) with the instance lock (self._lock) to serialize access to _proc, so that
other methods (health_check, generate) cannot mutate or observe _proc
concurrently; ensure you hold self._lock while checking if proc is None, calling
self._send({"op": "shutdown"}), waiting/terminating/killing the process, and
setting self._proc = None.

Comment on lines +302 to +342
slot_future = pool.submit(lambda: None)
try:
slot_future.result(timeout=10) # wait for our turn
except Exception:
slot_future.cancel()
raise

try:
with self._lock:
self._spawn()
msg = {"op": "synthesize", "text": text}
# Filter kwargs to JSON-safe primitives. Tensor / Path / etc.
# don't survive json.dumps and are silently dropped — the
# sidecar can't use them anyway.
for k, v in kw.items():
if _is_jsonable(v):
msg[k] = v
self._send(msg)
reply = self._recv_with_timeout(RECV_TIMEOUT_S)
if not reply:
raise RuntimeError(f"{self.id} sidecar closed pipe mid-generate")
if reply.get("op") == "error":
raise RuntimeError(
f"{self.id} sidecar error: {reply.get('message')!r}"
)
if reply.get("op") != "audio":
raise RuntimeError(
f"{self.id} sidecar returned unexpected op: {reply.get('op')!r}"
)
pcm_b64 = reply.get("audio_pcm_b64", "")
pcm = base64.b64decode(pcm_b64)
arr = np.frombuffer(pcm, dtype=np.int16).astype(np.float32) / 32768.0
tensor = torch.from_numpy(arr.copy()).unsqueeze(0)
return tensor
finally:
# Slot is released the instant this thread leaves the pool's
# task — by holding slot_future we kept one worker busy; nothing
# further to do. (ThreadPoolExecutor doesn't expose a manual
# release; the slot returns to the pool when our submitted no-op
# finishes, which happens immediately after .result() above.)
pass

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "Inspect GPU pool implementation and type:"
fd -i 'model_manager.py' --exec rg -nP '_get_gpu_pool|ThreadPoolExecutor|ProcessPoolExecutor|Semaphore' -C3 {}

echo
echo "Inspect current slot-acquire pattern in SubprocessBackend:"
rg -nP 'slot_future = pool\.submit\(lambda: None\)|slot_future\.result|def generate\(' -C4 backend/services/subprocess_backend.py

Repository: debpalash/OmniVoice-Studio

Length of output: 2693


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== Full generate() method implementation ==="
sed -n '287,342p' backend/services/subprocess_backend.py

Repository: debpalash/OmniVoice-Studio

Length of output: 2733


🏁 Script executed:

#!/bin/bash
set -euo pipefail

python3 << 'EOF'
import concurrent.futures
import time
import threading

# Simulate the current code pattern
pool = concurrent.futures.ThreadPoolExecutor(max_workers=2)
print("ThreadPoolExecutor with max_workers=2")

# Submit a no-op and immediately get result
future = pool.submit(lambda: None)
print(f"Task submitted, future state: {future}")

result = future.result(timeout=1)
print(f"After .result(): future completed, worker released back to pool")

# Now the main thread continues while worker is in the pool
print(f"Worker is now available for other tasks")
print(f"This is the exact pattern in SubprocessBackend.generate()")
EOF

Repository: debpalash/OmniVoice-Studio

Length of output: 360


GPU pool throttling is bypassed because worker is released before GPU synthesis begins.

Lines 302–304 submit a no-op task and wait for completion, which releases the ThreadPoolExecutor worker back to the pool immediately. All GPU synthesis work (lines 311–340) runs after the worker is released, defeating the stated intent to "hold GPU pool worker for the duration of this generate" (line 298–300). This allows unbounded concurrent synthesis instead of being throttled by max_workers.

💡 Suggested fix (hold worker for full generate lifecycle)
+        acquired = threading.Event()
+        release = threading.Event()
+
+        def _hold_slot() -> None:
+            acquired.set()
+            release.wait()
+
+        slot_future = pool.submit(_hold_slot)
         try:
-            slot_future.result(timeout=10)  # wait for our turn
+            if not acquired.wait(timeout=10):
+                raise TimeoutError("timed out acquiring GPU pool slot")
         except Exception:
             slot_future.cancel()
             raise

         try:
             with self._lock:
                 self._spawn()
                 msg = {"op": "synthesize", "text": text}
                 # Filter kwargs to JSON-safe primitives. Tensor / Path / etc.
                 # don't survive json.dumps and are silently dropped — the
                 # sidecar can't use them anyway.
                 for k, v in kw.items():
                     if _is_jsonable(v):
                         msg[k] = v
                 self._send(msg)
                 reply = self._recv_with_timeout(RECV_TIMEOUT_S)
             if not reply:
                 raise RuntimeError(f"{self.id} sidecar closed pipe mid-generate")
             if reply.get("op") == "error":
                 raise RuntimeError(
                     f"{self.id} sidecar error: {reply.get('message')!r}"
                 )
             if reply.get("op") != "audio":
                 raise RuntimeError(
                     f"{self.id} sidecar returned unexpected op: {reply.get('op')!r}"
                 )
             pcm_b64 = reply.get("audio_pcm_b64", "")
             pcm = base64.b64decode(pcm_b64)
             arr = np.frombuffer(pcm, dtype=np.int16).astype(np.float32) / 32768.0
             tensor = torch.from_numpy(arr.copy()).unsqueeze(0)
             return tensor
         finally:
+            release.set()
+            try:
+                slot_future.result(timeout=1)
+            except Exception:
+                logger.debug("[%s] GPU slot release join failed", self.id, exc_info=True)
-            # Slot is released the instant this thread leaves the pool's
-            # task — by holding slot_future we kept one worker busy; nothing
-            # further to do. (ThreadPoolExecutor doesn't expose a manual
-            # release; the slot returns to the pool when our submitted no-op
-            # finishes, which happens immediately after .result() above.)
-            pass
🤖 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/subprocess_backend.py` around lines 302 - 342, The current
pattern submits a no-op via pool.submit and .result() too early so the
ThreadPoolExecutor slot is released before GPU work starts; instead make the
submitted task block for the entire generate lifecycle and only complete in the
finally block so the worker remains reserved while you run self._spawn(),
_send(), and _recv_with_timeout(). Concretely: replace the immediate
no-op+result() with submitting a blocking callable (e.g., one that waits on a
threading.Event or similar) and keep that Event and the resulting slot_future
live while holding self._lock and performing synthesize (the code around
slot_future, self._spawn, self._send, and self._recv_with_timeout); in the
finally block set the Event (or otherwise unblock the callable) so the
slot_future can finish and return the worker to the pool. Ensure
slot_future.cancel() semantics are adjusted accordingly.

Comment on lines +362 to +394
def _recv(self) -> Optional[dict]:
"""Read one frame from the sidecar's stdout. Returns None on EOF.

Op allowlist is enforced here: unknown ops are logged and dropped,
and we tail-recurse to read the next frame. See T-02-04.
"""
if self._proc is None or self._proc.stdout is None:
return None
stdout = self._proc.stdout
header = _read_exact(stdout, 4)
if header is None:
return None
(n,) = struct.unpack("!I", header)
if n > MAX_FRAME_BYTES:
# T-02-01 — refuse to allocate before the body even arrives.
raise IOError(f"frame too large: {n}")
body = _read_exact(stdout, n)
if body is None or len(body) != n:
raise IOError("short read")
try:
msg = json.loads(body.decode("utf-8"))
except Exception as exc:
raise IOError(f"malformed sidecar frame: {exc}") from exc
op = msg.get("op") if isinstance(msg, dict) else None
if op not in PARENT_INBOUND_OPS:
# T-02-04 — refuse to act on unknown ops. Log and read the
# next frame so we don't desync.
logger.warning(
"[%s] dropped sidecar frame with disallowed op=%r",
self.id, op,
)
return self._recv()
return msg

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Replace recursive unknown-op skipping with an iterative loop.

Line 393 recursively calls _recv() for each dropped frame. A stream of disallowed ops can overflow recursion depth and crash the parent.

💡 Suggested fix
 def _recv(self) -> Optional[dict]:
@@
-    header = _read_exact(stdout, 4)
-    if header is None:
-        return None
-    (n,) = struct.unpack("!I", header)
-    if n > MAX_FRAME_BYTES:
-        # T-02-01 — refuse to allocate before the body even arrives.
-        raise IOError(f"frame too large: {n}")
-    body = _read_exact(stdout, n)
-    if body is None or len(body) != n:
-        raise IOError("short read")
-    try:
-        msg = json.loads(body.decode("utf-8"))
-    except Exception as exc:
-        raise IOError(f"malformed sidecar frame: {exc}") from exc
-    op = msg.get("op") if isinstance(msg, dict) else None
-    if op not in PARENT_INBOUND_OPS:
-        # T-02-04 — refuse to act on unknown ops. Log and read the
-        # next frame so we don't desync.
-        logger.warning(
-            "[%s] dropped sidecar frame with disallowed op=%r",
-            self.id, op,
-        )
-        return self._recv()
-    return msg
+    while True:
+        header = _read_exact(stdout, 4)
+        if header is None:
+            return None
+        (n,) = struct.unpack("!I", header)
+        if n > MAX_FRAME_BYTES:
+            raise IOError(f"frame too large: {n}")
+        body = _read_exact(stdout, n)
+        if body is None or len(body) != n:
+            raise IOError("short read")
+        try:
+            msg = json.loads(body.decode("utf-8"))
+        except Exception as exc:
+            raise IOError(f"malformed sidecar frame: {exc}") from exc
+        op = msg.get("op") if isinstance(msg, dict) else None
+        if op not in PARENT_INBOUND_OPS:
+            logger.warning(
+                "[%s] dropped sidecar frame with disallowed op=%r",
+                self.id, op,
+            )
+            continue
+        return msg
🤖 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/subprocess_backend.py` around lines 362 - 394, The _recv
method currently tail-recurses by returning self._recv() when a frame has an
unknown op, which can overflow the call stack; change this to an iterative loop:
wrap the frame-read/parse/validate sequence inside a while True loop that
continues reading the next frame when op not in PARENT_INBOUND_OPS (logging with
logger as before), and only return msg when op is allowed or return None/raise
on EOF/errors; keep uses of _read_exact, MAX_FRAME_BYTES, json.loads, and
self._proc.stdout intact and preserve existing error handling and logging.

Comment on lines +1217 to +1240
msg = f"{type(exc).__name__}: {exc}"
logger.warning(
"list_backends: %s.is_available() raised — degrading "
"gracefully so the picker still renders: %s",
bid, msg,
)
if ok:
_LAST_ERRORS.pop(bid, None)
else:
_LAST_ERRORS[bid] = msg
# ENGINE-06 isolation_mode: duck-typed marker for SubprocessBackend
# subclasses (see services.subprocess_backend.SubprocessBackend).
if getattr(cls, "_is_subprocess_isolated", False):
isolation = "subprocess"
else:
isolation = "in-process"
out.append({
"id": bid,
"display_name": cls.display_name,
"available": ok,
"reason": None if ok else msg,
"install_hint": _INSTALL_HINTS.get(bid),
"last_error": _LAST_ERRORS.get(bid),
"isolation_mode": isolation,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Sanitize exception text before persisting/exposing last_error and reason.

msg = f"{type(exc).__name__}: {exc}" is returned in API fields and cached, so raw exception strings can leak sensitive values (tokens/paths/internal endpoints) to the UI and logs.

Proposed fix
@@
-_LAST_ERRORS: dict[str, str] = {}
+_LAST_ERRORS: dict[str, str] = {}
+
+
+def _sanitize_error_message(msg: str, *, max_len: int = 512) -> str:
+    # Keep UI-safe diagnostics while reducing secret/path leakage risk.
+    redacted = msg
+    for key in ("HF_TOKEN", "HUGGINGFACEHUB_API_TOKEN", "OPENAI_API_KEY"):
+        val = os.environ.get(key)
+        if val:
+            redacted = redacted.replace(val, "***")
+    if len(redacted) > max_len:
+        redacted = redacted[:max_len] + "…"
+    return redacted
@@
-        except Exception as exc:
+        except Exception as exc:
             ok = False
-            msg = f"{type(exc).__name__}: {exc}"
+            msg = _sanitize_error_message(f"{type(exc).__name__}: {exc}")
🤖 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/tts_backend.py` around lines 1217 - 1240, The code currently
builds msg = f"{type(exc).__name__}: {exc}" and persists/returns it; change this
to keep the full msg for logging but create a sanitized_msg (e.g., only type
name and a generic note or truncated/escaped exception text without
paths/tokens) and use sanitized_msg when writing to _LAST_ERRORS and the out
dict fields "reason" and "last_error"; keep the original msg in logger.warning
or logger.debug for diagnostics but never persist or return it. Ensure you
update the assignment sites where _LAST_ERRORS[bId] and the out.append(...) use
msg to use sanitized_msg instead.

@debpalash
debpalash merged commit 0fc5ea6 into main May 20, 2026
8 checks passed
@debpalash
debpalash deleted the phase2-plan-02-01-subprocess-backend branch June 12, 2026 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant