Hartmut
71e099305c
- 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>
106 lines
9.1 KiB
Markdown
106 lines
9.1 KiB
Markdown
# Review Report: P2 USD Foundation
|
|
Date: 2026-03-12
|
|
|
|
## Result: ⚠️ Minor issues
|
|
|
|
---
|
|
|
|
## Problems Found
|
|
|
|
### [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.
|
|
|
|
---
|
|
|
|
### [backend/app/api/routers/cad.py:101,336] Hardcoded `"admin"` role string
|
|
**Severity**: Low
|
|
**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.
|
|
|
|
---
|
|
|
|
### [frontend/src/pages/Admin.tsx:42,50] `as any[]` for admin/users and templates queries
|
|
**Severity**: Low
|
|
**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 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
|
|
|
|
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.
|