fix: deduplicate GLB/USD generation with Redis locks + review fixes
- Add per-file Redis SET NX EX 1800 locks to generate_gltf_geometry_task and generate_usd_master_task — concurrent duplicates (e.g. double-click of bulk action buttons) now log a warning and return immediately instead of running two expensive OCC tessellation subprocesses on the same file - Fix eng.dispose() called inside with Session() block in cache-hit path of both tasks — moved to after the with block exits (Tasks 3+4 from plan) - Add cad.updated_at = datetime.utcnow() in save_manual_material_overrides (was missing vs parallel save_part_materials endpoint) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
+79
-24
@@ -1,4 +1,4 @@
|
||||
# Review Report: P1 Remaining Cleanup — M1 Dead Code + M3 blender_render.py Split
|
||||
# Review Report: P2 USD Foundation
|
||||
Date: 2026-03-12
|
||||
|
||||
## Result: ⚠️ Minor issues
|
||||
@@ -7,44 +7,99 @@ Date: 2026-03-12
|
||||
|
||||
## 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.
|
||||
### [backend/app/api/routers/cad.py:537] `save_manual_material_overrides` does not set `updated_at`
|
||||
**Severity**: Low
|
||||
**Recommendation**: Add `cad.updated_at = datetime.utcnow()` before `db.commit()`, consistent with `save_part_materials` at line 430 which does set it. The ORM `onupdate` on `CadFile.updated_at` only fires when SQLAlchemy detects a dirty column via ORM tracking. JSONB mutations are not always detected reliably.
|
||||
|
||||
---
|
||||
|
||||
### [step_processor.py:561] `_finalise_image()` ignores `fmt` parameter — always produces `.png`
|
||||
### [backend/app/api/routers/cad.py:101,336] Hardcoded `"admin"` role string
|
||||
**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.
|
||||
**Recommendation**: Lines 101 (`user.role.value != "admin"`) and 336 (`user.role.value != "admin"`) predate the multi-role system. These are not new in P2 but they remain in changed files. Should be migrated to `is_privileged(user)` / `require_admin()` on the next pass. Not a P2 blocker.
|
||||
|
||||
---
|
||||
|
||||
### [domains/rendering/tasks.py:210–215] New SQLAlchemy engine created per `render_turntable_task` call
|
||||
### [frontend/src/pages/Admin.tsx:42,50] `as any[]` for admin/users and templates queries
|
||||
**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.
|
||||
**Recommendation**: These two `as any[]` casts exist in pre-P2 code but are present in the changed file. Acceptable technical debt already present before P2; not introduced by P2 itself. Document for a future typing cleanup pass.
|
||||
|
||||
---
|
||||
|
||||
### [render-worker/scripts/export_step_to_usd.py:560] `schaeffler:partKey` stored as custom data, not a USD attribute
|
||||
**Severity**: Low (design note, not a bug)
|
||||
**Recommendation**: The implementation uses `prim.SetCustomDataByKey("schaeffler:partKey", ...)` which stores the value in the prim's `customData` dictionary rather than as a typed USD attribute. Both are valid USD patterns; `customData` is non-animatable metadata and is fine for a stable identifier used only by Python tooling. If downstream tools (e.g. Houdini LOPs) need to query it via `UsdAttributeQuery`, it would need `prim.CreateAttribute("schaeffler:partKey", Sdf.ValueTypeNames.String).Set(part_key)`. For the current use case the choice is acceptable.
|
||||
|
||||
---
|
||||
|
||||
### [render-worker/scripts/export_step_to_usd.py] No `UsdUtils.FlattenLayerStack()` call
|
||||
**Severity**: Low (checklist item — not applicable to this milestone)
|
||||
**Recommendation**: This script writes a single `.usd` layer (`Usd.Stage.CreateNew` + `stage.Save()`), so there are no sublayers to flatten. `FlattenLayerStack()` is only relevant when compositing multiple USDA/USDC layers into a delivery package. Not applicable at M1/M2 scope.
|
||||
|
||||
---
|
||||
|
||||
### [backend/app/core/tenant_context.py:70,100] New sync engine created per call
|
||||
**Severity**: Low (performance, not correctness)
|
||||
**Recommendation**: `resolve_tenant_id_for_cad()` and `resolve_tenant_id_for_order_line()` create a new `create_engine()` on every call and dispose it immediately, opening a fresh Postgres connection each time. At current task volume this is fine, but a module-level lazy singleton engine would be more efficient. Not a blocking issue for P2.
|
||||
|
||||
---
|
||||
|
||||
### [backend/app/domains/pipeline/tasks/export_glb.py:89-95] Confusing `eng.dispose()` placement in cache-hit early-return path
|
||||
**Severity**: Medium (code clarity; not a runtime bug)
|
||||
**Recommendation**: In `generate_gltf_geometry_task`, the cache-hit early-return path calls `eng.dispose()` and `return` from inside the `with Session(eng) as session:` block (lines 89-95). The Python context manager correctly closes the session before `dispose()` is reached, so this is safe. But the style is confusing — the normal path disposes the engine at line 96 after the `with` block, while the cache-hit path calls it from inside. If code is ever added after line 95, double-dispose risk increases. Recommend factoring the early-return check out of the `with` block.
|
||||
|
||||
---
|
||||
|
||||
### [frontend/src/components/cad/ThreeDViewer.tsx:536] `as any` cast for Three.js scene userData
|
||||
**Severity**: Low
|
||||
**Recommendation**: `(sceneRef.current as any).userData` is necessary due to the `@react-three/fiber` ref typing. A narrower cast `(sceneRef.current as THREE.Object3D).userData` would be more type-safe without losing any functionality.
|
||||
|
||||
---
|
||||
|
||||
## 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.
|
||||
- **All three migrations (060-062)** are correct, additive, and include proper `downgrade()` implementations. Migration 062 uses `UPDATE ... WHERE key = ...` safely — no unexpected DROP statements.
|
||||
- **USD exporter (`export_step_to_usd.py`)** is a comprehensive 631-line implementation: full XCAF traversal, correct OCC→USD Y-up coordinate swap (`(x, -z, y)`), index-space sharp-edge primvars stored against local mesh vertex indices (not world coordinates), `metersPerUnit=0.001` set correctly, and `MANIFEST_JSON:` stdout IPC protocol for the Celery task to parse without re-opening the USD file.
|
||||
- **`pxr` import is from `usd-core`** — correctly installed via `pip3 install "usd-core>=24.11"` in the Dockerfile.
|
||||
- **Celery task queue placement** is correct: both `generate_gltf_geometry_task` and `generate_usd_master_task` are on `thumbnail_rendering` (concurrency=1), ensuring no parallel Blender/OCC conflicts.
|
||||
- **`PipelineLogger` used** at the top of both new tasks with `self.request.id`. No `render_job_doc.celery_task_id` pattern needed here — that is specific to the `WorkflowRun` system; consistent with existing pipeline task patterns.
|
||||
- **Auth on all new endpoints**: `scene-manifest` (GET) requires `get_current_user`; `generate-usd-master` (POST) and `manual-material-overrides` (PUT) require `is_privileged(user)`. GET `manual-material-overrides` requires auth but not privilege — correct for read access.
|
||||
- **No SQL injections**: all DB access uses SQLAlchemy ORM or parameterized `text()` queries.
|
||||
- **Pydantic validation**: `ManualMaterialOverridesIn`, `PartEntry`, `SceneManifest` models validate all POST/PUT inputs.
|
||||
- **No hardcoded credentials or secrets** anywhere in the changed files.
|
||||
- **All English identifiers and comments** throughout.
|
||||
- **No `print()` in backend Python** — render-worker scripts use stdout as the IPC protocol with the Celery task, which is the correct approach.
|
||||
- **`storage_key` values are relative**: the `_prefix` strip logic in `export_glb.py` correctly strips `upload_dir` before storing.
|
||||
- **`_inject_glb_extras()` called after `RWGltf_CafWriter` export** — satisfies the render pipeline checklist item.
|
||||
- **Frontend types**: `SceneManifest`, `PartEntry`, `ManualMaterialOverridesResponse` all have proper TypeScript interfaces in `sceneManifest.ts` and `cad.ts`. No `as any` in the new API surface code.
|
||||
- **Loading states** with `isPending` and error feedback with `toast.error()` present in all new UI mutations.
|
||||
- **`effectiveMaterials` merge** in ThreeDViewer correctly combines legacy `partMaterials` (mesh-name keyed) and new `manualOverrides` (partKey keyed), providing backward compatibility for pre-P2 GLBs.
|
||||
- **`build_scene_manifest()` four-layer priority** (manual → auto → source → default) correctly implemented per spec.
|
||||
- **`generate_part_key()` algorithm duplicated consistently** between `export_step_to_usd.py` and `part_key_service.py` — same `_AF_RE` strip, same deduplication loop. Keys generated by the USD exporter match keys computed by the API service.
|
||||
- **New admin bulk-action endpoints** use `require_admin`, batch-fetch existing asset IDs efficiently (single query), and queue only missing items.
|
||||
- **`step_tasks.py` shim** correctly re-exports `generate_usd_master_task` for Celery task name resolution.
|
||||
- **New `tenant_context.py`** provides clean, documented sync helpers for RLS in Celery tasks, using parameterized `SET LOCAL` queries to prevent injection.
|
||||
- **`vite-env.d.ts`** fix (`/// <reference types="vite/client" />`) correctly resolves the `ImportMeta.env` TypeScript error without over-typing.
|
||||
- **`GPUProbeResult` interface extended** (`timestamp`, `devices`, `render_time_s`) without breaking existing callers — optional fields only.
|
||||
|
||||
---
|
||||
|
||||
## Summary of Issues
|
||||
|
||||
| # | File | Line | Severity | Description |
|
||||
|---|------|------|----------|-------------|
|
||||
| 1 | `cad.py` | 537 | Low | `updated_at` not set on `save_manual_material_overrides` |
|
||||
| 2 | `cad.py` | 101, 336 | Low | Pre-existing hardcoded `"admin"` role strings (not introduced by P2) |
|
||||
| 3 | `Admin.tsx` | 42, 50 | Low | Pre-existing `as any[]` casts (not introduced by P2) |
|
||||
| 4 | `export_step_to_usd.py` | 560 | Low | `schaeffler:partKey` in `customData` vs typed attribute (design choice, acceptable) |
|
||||
| 5 | `export_step_to_usd.py` | — | Low | No `FlattenLayerStack()` — not applicable at current scope |
|
||||
| 6 | `tenant_context.py` | 70, 100 | Low | New sync engine per call (performance concern only) |
|
||||
| 7 | `export_glb.py` | 89-95 | Medium | Confusing `eng.dispose()` in cache-hit early-return inside `with` block |
|
||||
| 8 | `ThreeDViewer.tsx` | 536 | Low | `as any` for scene userData (could use `THREE.Object3D`) |
|
||||
|
||||
No critical or blocking issues. All new endpoints are properly secured, migrations are clean, the USD exporter correctly authors index-space primvars and sets `schaeffler:partKey` metadata, and the frontend type coverage of the new API surface is complete.
|
||||
|
||||
---
|
||||
|
||||
## 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.
|
||||
Approved. Fix issue #1 (`updated_at` missing in `save_manual_material_overrides`) and issue #7 (refactor `eng.dispose()` in cache-hit path for clarity) before next production deployment. All remaining items are Low severity and can be addressed in a follow-up cleanup commit.
|
||||
|
||||
Reference in New Issue
Block a user