voicebox/docs/notes/BACKEND_CODE_REVIEW.md

Code Review: backend/ Post-Refactor

Date: 2026-03-16 Scope: Full review of backend/ after major refactor

Overall Assessment

The refactor is well-executed. The codebase follows a clean layered architecture (routes -> services -> backends) with good separation of concerns. The code is readable, the module boundaries are sensible, and the migration strategy is pragmatic for a desktop app. Below are findings organized by severity.

---

Critical Issues

1. Double init_db() in bundled entry point

File: server.py:261

server.py:261 calls database.init_db() explicitly, but app.py:140 also calls database.init_db() inside the startup event handler. When running via server.py, the database gets initialized twice -- once before uvicorn starts and once during the startup event. This is likely benign (idempotent migrations), but the second call recreates the engine and SessionLocal, which could cause subtle issues if any sessions were opened between the two calls.

Recommendation: Remove the explicit init_db() call in server.py:260-262 and rely solely on the startup event in app.py. The same issue exists in main.py:38.

2. SSE endpoint holds DB session open indefinitely

File: routes/generations.py:179-212

The get_generation_status SSE endpoint receives a db session via Depends(get_db) but keeps it open for the lifetime of the SSE stream (polling every 1 second). This ties up a SQLite connection for potentially minutes. With SQLite's single-writer model, this is a contention risk.

Recommendation: Open and close a short-lived session on each poll iteration instead of holding one via dependency injection:

async def event_stream():
    while True:
        db = next(get_db())
        try:
            gen = db.query(DBGeneration).filter_by(id=generation_id).first()
            ...
        finally:
            db.close()
        await asyncio.sleep(1)

---

High Severity

3. _save_retry creates no version record

File: services/generation.py:201-214

_save_retry writes the audio file but creates no GenerationVersion entry. If the generation previously had versions (from an initial generate that failed mid-effects, for example), the retry result won't appear in the versions list. This creates an inconsistency: some generations have versions, retried ones don't.

Recommendation: Create a "clean" version in _save_retry the same way _save_generate does.

4. datetime.utcnow() is deprecated

File: services/stories.py and others

datetime.utcnow() is deprecated as of Python 3.12 and returns a naive datetime. Used throughout services/stories.py (lines 95, 96, 193, 307, 360, 404, 457, 529, 537, 598, 610, 652, 716, 775) and possibly other service files.

Recommendation: Replace with datetime.now(datetime.UTC) or datetime.now(timezone.utc).

5. list_stories N+1 query

File: services/stories.py:122-132

list_stories issues one COUNT(*) query per story inside a loop. For N stories, that's N+1 queries.

Recommendation: Use a subquery or a single aggregated query:

from sqlalchemy import func
counts = dict(
    db.query(DBStoryItem.story_id, func.count(DBStoryItem.id))
    .group_by(DBStoryItem.story_id)
    .all()
)

---

Medium Severity

6. create_story queries item count immediately after creation

File: services/stories.py:103

Line 103 queries the item count for a story that was just created -- it will always be 0. This is wasted I/O.

7. Bare except Exception with silent pass

File: routes/generations.py:69-70

When parsing a profile's stored effects_chain JSON, exceptions are silently swallowed. A corrupt JSON blob would result in no effects being applied with no logging.

Recommendation: Log the exception at warning level.

8. update_story_item_times uses generation_id as key

File: services/stories.py:643-649

item_map is keyed by generation_id, but the same generation can appear in a story multiple times (via split/duplicate). This would cause key collisions, and only the last item per generation_id would be updatable.

Recommendation: Key by item_id instead, and change the StoryItemUpdateTime model to use item_id.

9. Thread safety gap in get_stt_backend

File: backends/__init__.py:499-520

get_stt_backend() uses no locking (unlike get_tts_backend_for_engine which uses _tts_backends_lock). A race condition could create duplicate STT backend instances.

Recommendation: Add a lock or use the same double-checked locking pattern.

10. Unused _tts_backend global

File: backends/__init__.py:156

_tts_backend is declared but never read or written outside of reset_backends(). All TTS access goes through _tts_backends dict. Dead code.

11. trim_story_item returns None for validation errors

File: services/stories.py:448

Returning None for "item not found" and "invalid trim values" is ambiguous. The route handler can't distinguish between a 404 and a 400 response.

Recommendation: Raise specific exceptions (e.g., ValueError for invalid trim) so the route can return the appropriate HTTP status.

12. load_engine_model calls different method names

File: backends/__init__.py:340-346

For Qwen, it calls load_model_async(model_size). For others, it calls load_model() with no arguments. But the TTSBackend protocol defines load_model(self, model_size: str). This means the protocol signature doesn't match actual usage for either path.

Recommendation: Align the protocol definition with actual backend implementations, or add load_model_async to the protocol.

---

Low Severity / Style

13. Inconsistent async usage in services

Functions like create_story, list_stories, etc. in services/stories.py are async def but contain no await expressions. They do synchronous SQLAlchemy I/O. While this works (the functions are awaitable), it's misleading -- these will block the event loop during DB access.

This is a known tradeoff with synchronous SQLAlchemy + FastAPI, and acceptable for a single-user desktop app with SQLite, but worth noting for documentation.

14. getattr(item, "version_id", None) pattern

File: services/stories.py:57, 504, 524, etc.

Multiple places use getattr(item, "version_id", None) on a DB model that has version_id as a declared column (from migrations). After the migration runs, this is always a real attribute. The defensive getattr is cargo-culted.

Recommendation: Access item.version_id directly. If the column is missing, the ORM will raise a clear error.

15. reorder_story_items ignores trim values

File: services/stories.py:707

When recalculating timecodes, it uses the full generation.duration rather than the effective (trimmed) duration. Trimmed items will have larger gaps than intended.

16. Module-level import torch in app.py:44

import torch at module level in app.py means torch loads on every import of the app module. This is intentional (AMD env vars must be set first), but the comment on line 38 should mention that this is why the import is here and not at the top.

17. f-strings in logging in server.py

server.py uses f-strings in logging calls (e.g., lines 63-66, 252, 256, 264). This evaluates the string even when the log level is filtered out. The rest of the codebase correctly uses %s style (e.g., app.py:131).

---

Architecture Observations (Not Issues)

• Clean layered design: routes -> services -> backends with Pydantic models as the API contract.
• Backend abstraction with Protocol classes and a config registry is a solid pattern.
• Serial generation queue (task_queue.py) is simple and effective for single-GPU serialization.
• Migration approach is pragmatic for the use case. The idempotent, check-then-act pattern is reliable.
• The generation.py refactor (collapsing three closures into run_generation with a mode parameter) is a clear improvement.

---

Summary

Severity	Count
Critical	2
High	3
Medium	7
Low/Style	5

The refactor achieved its goals: clear module boundaries, reduced duplication (especially in generation.py), and a well-organized backend abstraction. The critical items (double init_db and SSE session leak) should be addressed first, followed by the version consistency issue in retry and the N+1 query.