# Review Report: P1 Remaining Cleanup — M1 Dead Code + M3 blender_render.py Split
Date: 2026-03-12

## Result: ⚠️ Minor issues

---

## Problems Found

### [_blender_scene_setup.py:12] `import_usd_file` depends on uncommitted `_blender_import.py` change
**Severity**: Medium
**Description**: `_blender_scene_setup.py` imports `import_usd_file` from `_blender_import` (line 12). This function does NOT exist in the committed `_blender_import.py` at HEAD — it is only present in a pre-existing uncommitted modification to that file (`_blender_import.py` shows `??` is untracked/modified). If the new submodule files (`_blender_args.py`, `_blender_scene_setup.py`, `_blender_render_config.py`) are committed without including the `_blender_import.py` change, every Blender render will crash with `ImportError: cannot import name 'import_usd_file'`.
**Recommendation**: Ensure `render-worker/scripts/_blender_import.py` (with the `import_usd_file` addition) is staged and committed in the same commit as the new submodule files.

---

### [step_processor.py:561] `_finalise_image()` ignores `fmt` parameter — always produces `.png`
**Severity**: Low
**Description**: The new `_finalise_image(src, dst, fmt)` accepts `fmt` but always does `out = dst.with_suffix(".png")`. When `thumbnail_format = "jpg"` is configured in system settings, the callers compute `final_path` with a `.jpg` extension and pass `fmt="jpg"`, but the returned path will always have `.png` extension. The `fmt` parameter is silently ignored.

This is a deliberate consequence of removing PIL (which was never installed anyway), so the behavior change is acceptable. However, the dead `fmt` parameter creates misleading API.
**Recommendation**: Remove the `fmt` parameter from `_finalise_image()` and update all callers to drop it. This makes the intent clear: format conversion is not supported.

---

### [domains/rendering/tasks.py:210–215] New SQLAlchemy engine created per `render_turntable_task` call
**Severity**: Low
**Description**: `render_turntable_task` now calls `_create_engine(app_settings.database_url_sync)` inside the task body. This creates a new connection pool on every invocation. For a low-frequency task this is acceptable, but it leaks connection pool resources if the task runs frequently.
**Recommendation**: The `_db_engine` should be disposed after use. Add `_db_engine.dispose()` after the `with _Session(_db_engine) as _s:` block closes.

---

## Positives

- **All 7 plan tasks complete**: blender_render.py correctly reduced from 263 → 68 lines (meets ≤ 80 target).
- **`stl_quality` fully removed**: Zero matches in `render_blender.py`, `step_processor.py`, `domains/rendering/tasks.py` — clean sweep.
- **PIL fallback completely excised**: `_generate_thumbnail_placeholder()` and `from PIL import Image` both gone; the 68-line Pillow-placeholder function is deleted, reducing `step_processor.py` by ~70 lines.
- **Submodule separation is clean**: `_blender_args.py`, `_blender_scene_setup.py`, `_blender_render_config.py` follow the existing `_blender_gpu.py` / `_blender_import.py` / `_blender_scene.py` naming and import pattern exactly.
- **`_blender_args.py` fidelity**: All 25 positional args correctly mapped; named args `--mesh-attributes` and `--usd-path` preserved; `use_template` bool computed and included in namespace; template file existence check preserved.
- **`_blender_scene_setup.py` correctness**: MODE A and MODE B correctly separated; shadow catcher, lighting_only, auto-camera, and material library logic all preserved faithfully; `needs_auto_camera` conditional for MODE B correctly omits `setup_auto_lights` (template provides its own lights).
- **`_blender_render_config.py` correctness**: `configure_engine` signature matches exactly; colour management block (`Standard`/`None` look) correctly gated on `not use_template`; CYCLES verification block preserved.
- **`_lap` closure works correctly**: `_lap` is defined in `blender_render.py`, captures `_t0` and `_timings` from outer scope, passed as callback to both submodules — timing summary at end of `blender_render.py` correctly reflects labels from all phases.
- **`render_turntable_task` GLB path fixed**: Changed from `f"{stem}_{stl_quality}.glb"` (always `_low.glb`) to `{stem}_thumbnail.glb` — now consistent with `render_blender.py` service cache key.
- **Security**: No hardcoded credentials, no SQL injection vectors. Raw SQL in `render_turntable_task` is a parameterless `SELECT` with no user input.

---

## Recommendation

Fix the `_db_engine.dispose()` omission (low, 1 line) and commit `_blender_import.py` together with the new submodule files. The `fmt` parameter cleanup is optional polish. Fix inline and re-apply without re-review.