diff --git a/backend/app/api/routers/cad.py b/backend/app/api/routers/cad.py index 0a0928f..df8caa0 100644 --- a/backend/app/api/routers/cad.py +++ b/backend/app/api/routers/cad.py @@ -535,6 +535,7 @@ async def save_manual_material_overrides( cad = await _get_cad_file(id, db) cad.manual_material_overrides = body.overrides + cad.updated_at = datetime.utcnow() await db.commit() await db.refresh(cad) return ManualMaterialOverridesOut( diff --git a/backend/app/domains/pipeline/tasks/export_glb.py b/backend/app/domains/pipeline/tasks/export_glb.py index b958ed2..28de3be 100644 --- a/backend/app/domains/pipeline/tasks/export_glb.py +++ b/backend/app/domains/pipeline/tasks/export_glb.py @@ -24,191 +24,211 @@ def generate_gltf_geometry_task(self, cad_file_id: str): 4. Stores result as gltf_geometry MediaAsset (replaces any existing one) Output is in meters, Y-up (glTF convention). + A Redis dedup lock (TTL=30min) prevents concurrent duplicate tasks for the same file. """ import json as _json import os as _os import subprocess as _subprocess import sys as _sys + import redis as _redis_lib from pathlib import Path as _Path from sqlalchemy import create_engine, select as _select from sqlalchemy.orm import Session from app.config import settings as app_settings from app.models.cad_file import CadFile - from app.models.system_setting import SystemSetting as _SysSetting pl = PipelineLogger(task_id=self.request.id) pl.step_start("export_glb_geometry", {"cad_file_id": cad_file_id}) - # Resolve and log tenant context at task start (required for RLS) - from app.core.tenant_context import resolve_tenant_id_for_cad, set_tenant_context_sync - _tenant_id = resolve_tenant_id_for_cad(cad_file_id) + # Redis dedup lock: prevent concurrent duplicate tasks for the same cad_file_id + _lock_key = f"glb_geometry_lock:{cad_file_id}" + _r = _redis_lib.from_url(app_settings.redis_url) + _acquired = _r.set(_lock_key, "1", nx=True, ex=1800) # 30-min TTL + if not _acquired: + logger.warning("generate_gltf_geometry_task: %s already in-flight — skipping duplicate", cad_file_id) + pl.step_done("export_glb_geometry", result={"skipped": True, "reason": "duplicate"}) + return {"skipped": True} - sync_url = app_settings.database_url.replace("+asyncpg", "") - eng = create_engine(sync_url) - with Session(eng) as session: - set_tenant_context_sync(session, _tenant_id) - cad_file = session.get(CadFile, cad_file_id) - if not cad_file or not cad_file.stored_path: - logger.error("generate_gltf_geometry_task: no stored_path for %s", cad_file_id) - return - step_path_str = cad_file.stored_path + try: + # Resolve tenant context at task start (required for RLS) + from app.core.tenant_context import resolve_tenant_id_for_cad, set_tenant_context_sync + _tenant_id = resolve_tenant_id_for_cad(cad_file_id) - # Build hex color_map from product.cad_part_materials - from app.domains.products.models import Product - product = session.execute( - _select(Product).where(Product.cad_file_id == cad_file.id) - ).scalar_one_or_none() + sync_url = app_settings.database_url.replace("+asyncpg", "") + eng = create_engine(sync_url) + with Session(eng) as session: + set_tenant_context_sync(session, _tenant_id) + cad_file = session.get(CadFile, cad_file_id) + if not cad_file or not cad_file.stored_path: + logger.error("generate_gltf_geometry_task: no stored_path for %s", cad_file_id) + return + step_path_str = cad_file.stored_path - color_map: dict[str, str] = {} - product_id = str(product.id) if product else None - if product and product.cad_part_materials: - for entry in product.cad_part_materials: - part_name = entry.get("part_name") or entry.get("name", "") - hex_color = entry.get("hex_color") or entry.get("color", "") - if part_name and hex_color: - color_map[part_name] = hex_color + # Build hex color_map from product.cad_part_materials + from app.domains.products.models import Product + product = session.execute( + _select(Product).where(Product.cad_file_id == cad_file.id) + ).scalar_one_or_none() - settings_rows = session.execute(_select(_SysSetting)).scalars().all() - sys_settings = {s.key: s.value for s in settings_rows} + color_map: dict[str, str] = {} + product_id = str(product.id) if product else None + if product and product.cad_part_materials: + for entry in product.cad_part_materials: + part_name = entry.get("part_name") or entry.get("name", "") + hex_color = entry.get("hex_color") or entry.get("color", "") + if part_name and hex_color: + color_map[part_name] = hex_color - # Hash-based cache check: skip tessellation if file hasn't changed - step_file_hash = cad_file.step_file_hash - if step_file_hash: - from app.domains.media.models import MediaAsset, MediaAssetType - import uuid as _uuid_check - existing_geo = session.execute( - _select(MediaAsset).where( - MediaAsset.cad_file_id == _uuid_check.UUID(cad_file_id), + settings_rows = session.execute(_select(_SysSetting)).scalars().all() + sys_settings = {s.key: s.value for s in settings_rows} + + # Hash-based cache check: skip tessellation if file hasn't changed + step_file_hash = cad_file.step_file_hash + if step_file_hash: + from app.domains.media.models import MediaAsset, MediaAssetType + import uuid as _uuid_check + existing_geo = session.execute( + _select(MediaAsset).where( + MediaAsset.cad_file_id == _uuid_check.UUID(cad_file_id), + MediaAsset.asset_type == MediaAssetType.gltf_geometry, + ) + ).scalars().first() + if existing_geo: + logger.info("[CACHE] hash match — skipping geometry GLB tessellation for %s", cad_file_id) + pl.step_done("export_glb_geometry", result={"cached": True, "asset_id": str(existing_geo.id)}) + _cache_hit_asset_id = str(existing_geo.id) + else: + _cache_hit_asset_id = None + else: + _cache_hit_asset_id = None + eng.dispose() + + if _cache_hit_asset_id is not None: + # Still chain USD master — it has its own hash-check + try: + generate_usd_master_task.delay(cad_file_id) + except Exception: + logger.debug("Could not queue generate_usd_master_task from cache-hit path (non-fatal)") + return {"cached": True, "asset_id": _cache_hit_asset_id} + + linear_deflection = float(sys_settings.get("scene_linear_deflection", "0.1")) + angular_deflection = float(sys_settings.get("scene_angular_deflection", "0.1")) + tessellation_engine = sys_settings.get("tessellation_engine", "occ") + + step = _Path(step_path_str) + + if not step.exists(): + log_task_event(self.request.id, f"Failed: STEP file not found: {step}", "error") + raise RuntimeError(f"STEP file not found: {step}") + + output_path = step.parent / f"{step.stem}_geometry.glb" + + log_task_event( + self.request.id, + f"Starting OCC GLB export: {len(color_map)} part colors", + "info", + ) + + # Run export_step_to_gltf.py as a subprocess so OCP imports don't pollute worker state + scripts_dir = _Path(_os.environ.get("RENDER_SCRIPTS_DIR", "/render-scripts")) + script_path = scripts_dir / "export_step_to_gltf.py" + + python_bin = _sys.executable + cmd = [ + python_bin, str(script_path), + "--step_path", str(step), + "--output_path", str(output_path), + "--color_map", _json.dumps(color_map), + "--linear_deflection", str(linear_deflection), + "--angular_deflection", str(angular_deflection), + "--tessellation_engine", tessellation_engine, + ] + log_task_event( + self.request.id, + f"Tessellation ({tessellation_engine}): linear={linear_deflection}mm, angular={angular_deflection}rad", + "info", + ) + + try: + result = _subprocess.run(cmd, capture_output=True, text=True, timeout=600) + for line in result.stdout.splitlines(): + logger.info("[occ-gltf] %s", line) + for line in result.stderr.splitlines(): + logger.warning("[occ-gltf stderr] %s", line) + + if result.returncode != 0 or not output_path.exists() or output_path.stat().st_size == 0: + raise RuntimeError( + f"export_step_to_gltf.py failed (exit {result.returncode}).\n" + f"STDERR: {result.stderr[-1000:]}" + ) + except Exception as exc: + log_task_event(self.request.id, f"Failed: {exc}", "error") + pl.step_error("export_glb_geometry", str(exc), exc) + logger.error("generate_gltf_geometry_task OCC export failed: %s", exc) + raise self.retry(exc=exc, countdown=15) + + log_task_event(self.request.id, f"OCC GLB export completed: {output_path.name}", "done") + + # --- Store MediaAsset (upsert: update existing to keep stable ID/URL) --- + import uuid as _uuid + from sqlalchemy import create_engine as _ce, select as _sel2 + from sqlalchemy.orm import Session as _Session + from app.domains.media.models import MediaAsset, MediaAssetType + + _sync_url = app_settings.database_url.replace("+asyncpg", "") + _eng2 = _ce(_sync_url) + with _Session(_eng2) as _sess: + set_tenant_context_sync(_sess, _tenant_id) + _key = str(output_path) + _prefix = str(app_settings.upload_dir).rstrip("/") + "/" + if _key.startswith(_prefix): + _key = _key[len(_prefix):] + _file_size = output_path.stat().st_size if output_path.exists() else None + + existing = _sess.execute( + _sel2(MediaAsset).where( + MediaAsset.cad_file_id == _uuid.UUID(cad_file_id), MediaAsset.asset_type == MediaAssetType.gltf_geometry, ) ).scalars().first() - if existing_geo: - logger.info("[CACHE] hash match — skipping geometry GLB tessellation for %s", cad_file_id) - pl.step_done("export_glb_geometry", result={"cached": True, "asset_id": str(existing_geo.id)}) - eng.dispose() - # Still chain USD master — it has its own hash-check (C2) - try: - generate_usd_master_task.delay(cad_file_id) - except Exception: - logger.debug("Could not queue generate_usd_master_task from cache-hit path (non-fatal)") - return {"cached": True, "asset_id": str(existing_geo.id)} - eng.dispose() - linear_deflection = float(sys_settings.get("scene_linear_deflection", "0.1")) - angular_deflection = float(sys_settings.get("scene_angular_deflection", "0.1")) - tessellation_engine = sys_settings.get("tessellation_engine", "occ") + if existing: + existing.storage_key = _key + existing.mime_type = "model/gltf-binary" + existing.file_size_bytes = _file_size + if product_id: + existing.product_id = _uuid.UUID(product_id) + _sess.commit() + asset_id = str(existing.id) + else: + asset = MediaAsset( + cad_file_id=_uuid.UUID(cad_file_id), + product_id=_uuid.UUID(product_id) if product_id else None, + asset_type=MediaAssetType.gltf_geometry, + storage_key=_key, + mime_type="model/gltf-binary", + file_size_bytes=_file_size, + ) + _sess.add(asset) + _sess.commit() + asset_id = str(asset.id) + _eng2.dispose() - step = _Path(step_path_str) + pl.step_done("export_glb_geometry", result={"glb_path": str(output_path), "asset_id": asset_id}) + logger.info("generate_gltf_geometry_task: MediaAsset %s created for cad %s", asset_id, cad_file_id) - if not step.exists(): - log_task_event(self.request.id, f"Failed: STEP file not found: {step}", "error") - raise RuntimeError(f"STEP file not found: {step}") + # Auto-chain USD master export so the canonical scene is always up to date + try: + generate_usd_master_task.delay(cad_file_id) + logger.info("generate_gltf_geometry_task: queued generate_usd_master_task for %s", cad_file_id) + except Exception: + logger.debug("Could not queue generate_usd_master_task (non-fatal)") - output_path = step.parent / f"{step.stem}_geometry.glb" + return {"glb_path": str(output_path), "asset_id": asset_id} - log_task_event( - self.request.id, - f"Starting OCC GLB export: {len(color_map)} part colors", - "info", - ) - - # Run export_step_to_gltf.py as a subprocess so OCP imports don't pollute worker state - scripts_dir = _Path(_os.environ.get("RENDER_SCRIPTS_DIR", "/render-scripts")) - script_path = scripts_dir / "export_step_to_gltf.py" - - python_bin = _sys.executable - cmd = [ - python_bin, str(script_path), - "--step_path", str(step), - "--output_path", str(output_path), - "--color_map", _json.dumps(color_map), - "--linear_deflection", str(linear_deflection), - "--angular_deflection", str(angular_deflection), - "--tessellation_engine", tessellation_engine, - ] - log_task_event( - self.request.id, - f"Tessellation ({tessellation_engine}): linear={linear_deflection}mm, angular={angular_deflection}rad", - "info", - ) - - try: - result = _subprocess.run(cmd, capture_output=True, text=True, timeout=600) - for line in result.stdout.splitlines(): - logger.info("[occ-gltf] %s", line) - for line in result.stderr.splitlines(): - logger.warning("[occ-gltf stderr] %s", line) - - if result.returncode != 0 or not output_path.exists() or output_path.stat().st_size == 0: - raise RuntimeError( - f"export_step_to_gltf.py failed (exit {result.returncode}).\n" - f"STDERR: {result.stderr[-1000:]}" - ) - except Exception as exc: - log_task_event(self.request.id, f"Failed: {exc}", "error") - pl.step_error("export_glb_geometry", str(exc), exc) - logger.error("generate_gltf_geometry_task OCC export failed: %s", exc) - raise self.retry(exc=exc, countdown=15) - - log_task_event(self.request.id, f"OCC GLB export completed: {output_path.name}", "done") - - # --- Store MediaAsset (upsert: update existing to keep stable ID/URL) --- - import uuid as _uuid - from sqlalchemy import create_engine as _ce, select as _sel2 - from sqlalchemy.orm import Session as _Session - from app.domains.media.models import MediaAsset, MediaAssetType - - _sync_url = app_settings.database_url.replace("+asyncpg", "") - _eng2 = _ce(_sync_url) - with _Session(_eng2) as _sess: - set_tenant_context_sync(_sess, _tenant_id) - _key = str(output_path) - _prefix = str(app_settings.upload_dir).rstrip("/") + "/" - if _key.startswith(_prefix): - _key = _key[len(_prefix):] - _file_size = output_path.stat().st_size if output_path.exists() else None - - existing = _sess.execute( - _sel2(MediaAsset).where( - MediaAsset.cad_file_id == _uuid.UUID(cad_file_id), - MediaAsset.asset_type == MediaAssetType.gltf_geometry, - ) - ).scalars().first() - - if existing: - existing.storage_key = _key - existing.mime_type = "model/gltf-binary" - existing.file_size_bytes = _file_size - if product_id: - existing.product_id = _uuid.UUID(product_id) - _sess.commit() - asset_id = str(existing.id) - else: - asset = MediaAsset( - cad_file_id=_uuid.UUID(cad_file_id), - product_id=_uuid.UUID(product_id) if product_id else None, - asset_type=MediaAssetType.gltf_geometry, - storage_key=_key, - mime_type="model/gltf-binary", - file_size_bytes=_file_size, - ) - _sess.add(asset) - _sess.commit() - asset_id = str(asset.id) - _eng2.dispose() - - pl.step_done("export_glb_geometry", result={"glb_path": str(output_path), "asset_id": asset_id}) - logger.info("generate_gltf_geometry_task: MediaAsset %s created for cad %s", asset_id, cad_file_id) - - # Auto-chain USD master export so the canonical scene is always up to date - try: - generate_usd_master_task.delay(cad_file_id) - logger.info("generate_gltf_geometry_task: queued generate_usd_master_task for %s", cad_file_id) - except Exception: - logger.debug("Could not queue generate_usd_master_task (non-fatal)") - - return {"glb_path": str(output_path), "asset_id": asset_id} + finally: + _r.delete(_lock_key) @celery_app.task( @@ -521,169 +541,186 @@ def generate_usd_master_task(self, cad_file_id: str) -> dict: pl = PipelineLogger(task_id=self.request.id) pl.step_start("usd_master", {"cad_file_id": cad_file_id}) - from app.core.tenant_context import resolve_tenant_id_for_cad, set_tenant_context_sync - _tenant_id = resolve_tenant_id_for_cad(cad_file_id) + # Redis dedup lock: prevent concurrent duplicate tasks for the same cad_file_id + import redis as _redis_lib + _lock_key = f"usd_master_lock:{cad_file_id}" + _r = _redis_lib.from_url(app_settings.redis_url) + _acquired = _r.set(_lock_key, "1", nx=True, ex=1800) # 30-min TTL + if not _acquired: + logger.warning("generate_usd_master_task: %s already in-flight — skipping duplicate", cad_file_id) + pl.step_done("usd_master", result={"skipped": True, "reason": "duplicate"}) + return {"skipped": True} - sync_url = app_settings.database_url.replace("+asyncpg", "") - eng = _ce(sync_url) + try: + from app.core.tenant_context import resolve_tenant_id_for_cad, set_tenant_context_sync + _tenant_id = resolve_tenant_id_for_cad(cad_file_id) - with _Session(eng) as sess: - set_tenant_context_sync(sess, _tenant_id) - cad_file = sess.get(CadFile, cad_file_id) - if not cad_file or not cad_file.stored_path: - logger.error("generate_usd_master_task: no stored_path for %s", cad_file_id) - return {"error": "no stored_path"} + sync_url = app_settings.database_url.replace("+asyncpg", "") + eng = _ce(sync_url) + _cache_hit_asset_id: str | None = None - step_path = _Path(cad_file.stored_path) + with _Session(eng) as sess: + set_tenant_context_sync(sess, _tenant_id) + cad_file = sess.get(CadFile, cad_file_id) + if not cad_file or not cad_file.stored_path: + logger.error("generate_usd_master_task: no stored_path for %s", cad_file_id) + return {"error": "no stored_path"} - product = sess.execute( - _sel(Product).where(Product.cad_file_id == cad_file.id) - ).scalar_one_or_none() + step_path = _Path(cad_file.stored_path) - color_map: dict[str, str] = {} - if product and product.cad_part_materials: - for entry in product.cad_part_materials: - part_name = entry.get("part_name") or entry.get("name", "") - hex_color = entry.get("hex_color") or entry.get("color", "") - if part_name and hex_color: - color_map[part_name] = hex_color + product = sess.execute( + _sel(Product).where(Product.cad_file_id == cad_file.id) + ).scalar_one_or_none() - settings_rows = sess.execute(_sel(SystemSetting)).scalars().all() - sys_settings = {s.key: s.value for s in settings_rows} + color_map: dict[str, str] = {} + if product and product.cad_part_materials: + for entry in product.cad_part_materials: + part_name = entry.get("part_name") or entry.get("name", "") + hex_color = entry.get("hex_color") or entry.get("color", "") + if part_name and hex_color: + color_map[part_name] = hex_color - # Hash-based cache check: skip tessellation if file hasn't changed - step_file_hash = cad_file.step_file_hash - if step_file_hash: - existing_usd = sess.execute( + settings_rows = sess.execute(_sel(SystemSetting)).scalars().all() + sys_settings = {s.key: s.value for s in settings_rows} + + # Hash-based cache check: skip tessellation if file hasn't changed + step_file_hash = cad_file.step_file_hash + if step_file_hash: + existing_usd = sess.execute( + _sel(MediaAsset).where( + MediaAsset.cad_file_id == cad_file.id, + MediaAsset.asset_type == MediaAssetType.usd_master, + ) + ).scalars().first() + if existing_usd: + logger.info("[CACHE] hash match — skipping USD master tessellation for %s", cad_file_id) + pl.step_done("usd_master", result={"cached": True, "asset_id": str(existing_usd.id)}) + _cache_hit_asset_id = str(existing_usd.id) + eng.dispose() + + if _cache_hit_asset_id is not None: + return {"cached": True, "asset_id": _cache_hit_asset_id} + + if not step_path.exists(): + err = f"STEP file not found: {step_path}" + pl.step_error("usd_master", err, None) + raise RuntimeError(err) + + linear_deflection = float(sys_settings.get("render_linear_deflection", "0.03")) + angular_deflection = float(sys_settings.get("render_angular_deflection", "0.05")) + sharp_threshold = float(sys_settings.get("sharp_edge_threshold", "20.0")) + + output_path = step_path.parent / f"{step_path.stem}_master.usd" + scripts_dir = _Path(_os.environ.get("RENDER_SCRIPTS_DIR", "/render-scripts")) + script_path = scripts_dir / "export_step_to_usd.py" + + if not script_path.exists(): + err = f"export_step_to_usd.py not found at {script_path}" + pl.step_error("usd_master", err, None) + raise RuntimeError(err) + + cmd = [ + _sys.executable, str(script_path), + "--step_path", str(step_path), + "--output_path", str(output_path), + "--color_map", _json.dumps(color_map), + "--linear_deflection", str(linear_deflection), + "--angular_deflection", str(angular_deflection), + "--sharp_threshold", str(sharp_threshold), + "--cad_file_id", cad_file_id, + ] + + log_task_event( + self.request.id, + f"[USD_MASTER] exporting STEP → USD: {step_path.name}", + "info", + ) + + try: + result = _subprocess.run(cmd, capture_output=True, text=True, timeout=600) + for line in result.stdout.splitlines(): + logger.info("[usd-master] %s", line) + for line in result.stderr.splitlines(): + logger.warning("[usd-master stderr] %s", line) + + if result.returncode != 0 or not output_path.exists() or output_path.stat().st_size == 0: + raise RuntimeError( + f"export_step_to_usd.py failed (exit {result.returncode}).\n" + f"STDERR: {result.stderr[-1000:]}" + ) + except Exception as exc: + log_task_event(self.request.id, f"[USD_MASTER] failed: {exc}", "error") + pl.step_error("usd_master", str(exc), exc) + raise self.retry(exc=exc, countdown=15) + + # --- Store MediaAsset (upsert) --- + eng2 = _ce(sync_url) + asset_id: str = "" + with _Session(eng2) as sess2: + set_tenant_context_sync(sess2, _tenant_id) + _key = str(output_path) + _prefix = str(app_settings.upload_dir).rstrip("/") + "/" + if _key.startswith(_prefix): + _key = _key[len(_prefix):] + _file_size = output_path.stat().st_size if output_path.exists() else None + + existing = sess2.execute( _sel(MediaAsset).where( - MediaAsset.cad_file_id == cad_file.id, + MediaAsset.cad_file_id == _uuid.UUID(cad_file_id), MediaAsset.asset_type == MediaAssetType.usd_master, ) ).scalars().first() - if existing_usd: - logger.info("[CACHE] hash match — skipping USD master tessellation for %s", cad_file_id) - pl.step_done("usd_master", result={"cached": True, "asset_id": str(existing_usd.id)}) - eng.dispose() - return {"cached": True, "asset_id": str(existing_usd.id)} - eng.dispose() - if not step_path.exists(): - err = f"STEP file not found: {step_path}" - pl.step_error("usd_master", err, None) - raise RuntimeError(err) + if existing: + existing.storage_key = _key + existing.mime_type = "model/vnd.usd" + existing.file_size_bytes = _file_size + sess2.commit() + asset_id = str(existing.id) + else: + asset = MediaAsset( + cad_file_id=_uuid.UUID(cad_file_id), + asset_type=MediaAssetType.usd_master, + storage_key=_key, + mime_type="model/vnd.usd", + file_size_bytes=_file_size, + ) + sess2.add(asset) + sess2.commit() + asset_id = str(asset.id) + eng2.dispose() - linear_deflection = float(sys_settings.get("render_linear_deflection", "0.03")) - angular_deflection = float(sys_settings.get("render_angular_deflection", "0.05")) - sharp_threshold = float(sys_settings.get("sharp_edge_threshold", "20.0")) - - output_path = step_path.parent / f"{step_path.stem}_master.usd" - scripts_dir = _Path(_os.environ.get("RENDER_SCRIPTS_DIR", "/render-scripts")) - script_path = scripts_dir / "export_step_to_usd.py" - - if not script_path.exists(): - err = f"export_step_to_usd.py not found at {script_path}" - pl.step_error("usd_master", err, None) - raise RuntimeError(err) - - cmd = [ - _sys.executable, str(script_path), - "--step_path", str(step_path), - "--output_path", str(output_path), - "--color_map", _json.dumps(color_map), - "--linear_deflection", str(linear_deflection), - "--angular_deflection", str(angular_deflection), - "--sharp_threshold", str(sharp_threshold), - "--cad_file_id", cad_file_id, - ] - - log_task_event( - self.request.id, - f"[USD_MASTER] exporting STEP → USD: {step_path.name}", - "info", - ) - - try: - result = _subprocess.run(cmd, capture_output=True, text=True, timeout=600) + # --- Parse MANIFEST_JSON and write resolved_material_assignments --- + manifest_parts: list = [] for line in result.stdout.splitlines(): - logger.info("[usd-master] %s", line) - for line in result.stderr.splitlines(): - logger.warning("[usd-master stderr] %s", line) + if line.startswith("MANIFEST_JSON: "): + try: + manifest_parts = _json.loads(line[len("MANIFEST_JSON: "):]).get("parts", []) + except Exception as parse_exc: + logger.warning("[USD_MASTER] MANIFEST_JSON parse failed: %s", parse_exc) + break - if result.returncode != 0 or not output_path.exists() or output_path.stat().st_size == 0: - raise RuntimeError( - f"export_step_to_usd.py failed (exit {result.returncode}).\n" - f"STDERR: {result.stderr[-1000:]}" - ) - except Exception as exc: - log_task_event(self.request.id, f"[USD_MASTER] failed: {exc}", "error") - pl.step_error("usd_master", str(exc), exc) - raise self.retry(exc=exc, countdown=15) - - # --- Store MediaAsset (upsert) --- - eng2 = _ce(sync_url) - asset_id: str = "" - with _Session(eng2) as sess2: - set_tenant_context_sync(sess2, _tenant_id) - _key = str(output_path) - _prefix = str(app_settings.upload_dir).rstrip("/") + "/" - if _key.startswith(_prefix): - _key = _key[len(_prefix):] - _file_size = output_path.stat().st_size if output_path.exists() else None - - existing = sess2.execute( - _sel(MediaAsset).where( - MediaAsset.cad_file_id == _uuid.UUID(cad_file_id), - MediaAsset.asset_type == MediaAssetType.usd_master, - ) - ).scalars().first() - - if existing: - existing.storage_key = _key - existing.mime_type = "model/vnd.usd" - existing.file_size_bytes = _file_size - sess2.commit() - asset_id = str(existing.id) - else: - asset = MediaAsset( - cad_file_id=_uuid.UUID(cad_file_id), - asset_type=MediaAssetType.usd_master, - storage_key=_key, - mime_type="model/vnd.usd", - file_size_bytes=_file_size, - ) - sess2.add(asset) - sess2.commit() - asset_id = str(asset.id) - eng2.dispose() - - # --- Parse MANIFEST_JSON and write resolved_material_assignments --- - manifest_parts: list = [] - for line in result.stdout.splitlines(): - if line.startswith("MANIFEST_JSON: "): + if manifest_parts: try: - manifest_parts = _json.loads(line[len("MANIFEST_JSON: "):]).get("parts", []) - except Exception as parse_exc: - logger.warning("[USD_MASTER] MANIFEST_JSON parse failed: %s", parse_exc) - break + resolved = { + p["part_key"]: {"source_name": p["source_name"], "prim_path": p["prim_path"]} + for p in manifest_parts + } + eng3 = _ce(sync_url) + with _Session(eng3) as sess3: + set_tenant_context_sync(sess3, _tenant_id) + row = sess3.get(CadFile, cad_file_id) + if row: + row.resolved_material_assignments = resolved + sess3.commit() + eng3.dispose() + logger.info("[USD_MASTER] wrote resolved_material_assignments (%d parts)", len(resolved)) + except Exception as write_exc: + logger.warning("[USD_MASTER] failed to write resolved_material_assignments: %s", write_exc) - if manifest_parts: - try: - resolved = { - p["part_key"]: {"source_name": p["source_name"], "prim_path": p["prim_path"]} - for p in manifest_parts - } - eng3 = _ce(sync_url) - with _Session(eng3) as sess3: - set_tenant_context_sync(sess3, _tenant_id) - row = sess3.get(CadFile, cad_file_id) - if row: - row.resolved_material_assignments = resolved - sess3.commit() - eng3.dispose() - logger.info("[USD_MASTER] wrote resolved_material_assignments (%d parts)", len(resolved)) - except Exception as write_exc: - logger.warning("[USD_MASTER] failed to write resolved_material_assignments: %s", write_exc) + log_task_event(self.request.id, f"[USD_MASTER] done: {output_path.name}", "done") + pl.step_done("usd_master", result={"usd_path": str(output_path), "asset_id": asset_id}) + return {"usd_path": str(output_path), "asset_id": asset_id, "n_parts": len(manifest_parts)} - log_task_event(self.request.id, f"[USD_MASTER] done: {output_path.name}", "done") - pl.step_done("usd_master", result={"usd_path": str(output_path), "asset_id": asset_id}) - return {"usd_path": str(output_path), "asset_id": asset_id, "n_parts": len(manifest_parts)} + finally: + _r.delete(_lock_key) diff --git a/plan.md b/plan.md index 1c74173..688424a 100644 --- a/plan.md +++ b/plan.md @@ -1,108 +1,150 @@ -# Plan: P2 USD Foundation — Commit & Verify +# Plan: Deduplication for GLB/USD Generation + Two Review Fixes ## Context -All five P2 milestones are already implemented in the working tree as uncommitted changes. -The task now is to apply the DB migrations, commit the work, and verify the stack runs. +Two problems to solve: -### Milestone status (assessed 2026-03-12) +**1. Duplicate generation (main bug)** +When "Generate Missing Canonical Scenes" or "Generate Missing USD Masters" is clicked, the admin endpoint queries for CAD files without a `gltf_geometry` / `usd_master` MediaAsset and queues one task per file. If the button is clicked twice (or both endpoints are triggered in sequence before any task has committed its MediaAsset), the same `cad_file_id` is queued multiple times. The tasks also auto-chain: `generate_gltf_geometry_task` always queues `generate_usd_master_task` at the end — so clicking "Generate Missing USD Masters" while GLB tasks are still running doubles up the USD work. -| Milestone | Status | Key files | -|---|---|---| -| M1: `export_step_to_usd.py` with `schaeffler:partKey` | ✅ DONE | `render-worker/scripts/export_step_to_usd.py` (631 lines) | -| M2: `usd_master` MediaAsset + migrations 060–062 + Celery task | ✅ DONE | migrations 060/061/062, `generate_usd_master_task` in `export_glb.py` | -| M3: `GET /api/cad/{id}/scene-manifest` | ✅ DONE | `part_key_service.py`, `SceneManifest` schema, endpoint in `cad.py` | -| M4: `PUT /api/cad/{id}/manual-material-overrides` | ✅ DONE | New endpoint pair in `cad.py`, `saveManualOverrides` in `cad.ts` | -| M5: ThreeDViewer uses partKey, survives reload | ✅ DONE | `partKeyMap` in GLB extras, `effectiveMaterials` merge, server-side persistence | +The existing cache check (`step_file_hash`) only short-circuits tessellation when a MediaAsset already exists — it does not prevent two concurrent tasks from both starting the expensive subprocess on the same file. Two processes writing to `_geometry.glb` simultaneously causes corruption / wasted compute. -## Affected Files (all uncommitted — working tree only) +**Solution**: Apply the same Redis `SET NX EX` dedup lock that `process_step_file` uses (lock key `step_processing_lock:{id}`, released in `finally`). Add equivalent locks to `generate_gltf_geometry_task` and `generate_usd_master_task`. -**Backend** -- `backend/alembic/versions/060_usd_master_asset_type.py` — new migration -- `backend/alembic/versions/061_material_assignment_layers.py` — new migration -- `backend/alembic/versions/062_rename_tessellation_settings.py` — new migration -- `backend/app/domains/media/models.py` — `MediaAssetType.usd_master` added -- `backend/app/domains/products/models.py` — 3 new JSONB columns on `CadFile` -- `backend/app/domains/products/schemas.py` — `SceneManifest`, `PartEntry` Pydantic models -- `backend/app/domains/pipeline/tasks/export_glb.py` — `generate_usd_master_task` + auto-chain -- `backend/app/domains/pipeline/tasks/extract_metadata.py` — minor update -- `backend/app/domains/pipeline/tasks/render_thumbnail.py` — minor update -- `backend/app/domains/pipeline/tasks/render_order_line.py` — minor update -- `backend/app/api/routers/cad.py` — scene-manifest + manual-material-overrides endpoints -- `backend/app/api/routers/admin.py` — generate-missing-usd-masters + generate-missing-canonical-scenes buttons -- `backend/app/services/part_key_service.py` — new file: `build_scene_manifest()`, `generate_part_key()` -- `backend/app/core/config_service.py` — minor update -- `backend/app/core/tenant_context.py` — new file -- `backend/app/tasks/step_tasks.py` — re-exports `generate_usd_master_task` +**2. Review fix A — `eng.dispose()` inside `with Session` block** +`export_glb.py` line 89: `eng.dispose()` is called inside the `with Session(eng)` context manager before the `return`. The context manager's `__exit__` then tries to close a session on a disposed engine. Safe in practice (no exception raised) but fragile and misleading. Move `eng.dispose()` to after the `with` block exits. -**Render worker** -- `render-worker/scripts/export_step_to_usd.py` — new file: full USD exporter -- `render-worker/scripts/export_step_to_gltf.py` — injects `partKeyMap` into GLB extras -- `render-worker/scripts/still_render.py` — USD path support -- `render-worker/scripts/turntable_render.py` — USD path support -- `render-worker/Dockerfile` — `usd-core>=24.11` added +**3. Review fix B — `save_manual_material_overrides` missing `updated_at`** +`cad.py` line 537: `cad.manual_material_overrides = body.overrides` is committed without updating `cad.updated_at`. The parallel endpoint `save_part_materials` (line 430) does call `cad.updated_at = datetime.utcnow()`. Add the same line to `save_manual_material_overrides`. -**Frontend** -- `frontend/src/api/cad.ts` — `getManualOverrides()`, `saveManualOverrides()` -- `frontend/src/api/media.ts` — `usd_master` type added -- `frontend/src/api/sceneManifest.ts` — new file: `SceneManifest`, `fetchSceneManifest()` -- `frontend/src/components/cad/ThreeDViewer.tsx` — `partKeyMap`, `effectiveMaterials`, reconciliation panel -- `frontend/src/components/cad/MaterialPanel.tsx` — dual-path save, provenance badge -- `frontend/src/pages/Admin.tsx` — USD master bulk action buttons -- `frontend/src/pages/ProductDetail.tsx` — `usd_master` row in asset table -- `frontend/src/pages/Orders.tsx` — minor update +## Affected Files + +| File | Change | +|---|---| +| `backend/app/domains/pipeline/tasks/export_glb.py` | Add Redis dedup locks to `generate_gltf_geometry_task` and `generate_usd_master_task`; fix `eng.dispose()` placement | +| `backend/app/api/routers/cad.py` | Add `cad.updated_at = datetime.utcnow()` in `save_manual_material_overrides` | ## Tasks (in order) -### [ ] Task 1: Apply migrations 060–062 -- **What**: Run `docker compose exec backend alembic upgrade head` to apply the three pending migrations -- **Acceptance gate**: `docker compose exec backend alembic current` shows `062` (or higher) as current +### [x] Task 1: Add Redis dedup locks to `generate_gltf_geometry_task` + +- **File**: `backend/app/domains/pipeline/tasks/export_glb.py` +- **What**: At the top of `generate_gltf_geometry_task`, after `pl.step_start(...)`, acquire a Redis lock using the same pattern as `extract_metadata.py`: + + ```python + import redis as _redis_lib + _lock_key = f"glb_geometry_lock:{cad_file_id}" + _r = _redis_lib.from_url(app_settings.redis_url) + _acquired = _r.set(_lock_key, "1", nx=True, ex=1800) # 30-min TTL + if not _acquired: + logger.warning("generate_gltf_geometry_task: %s already in-flight — skipping duplicate", cad_file_id) + pl.step_done("export_glb_geometry", result={"skipped": True, "reason": "duplicate"}) + return {"skipped": True} + ``` + + Wrap the rest of the task body in `try: ... finally: _r.delete(_lock_key)`. + + Note: `app_settings` is already imported inside the function. Import `redis` at the top of the `try` block as `import redis as _redis_lib` (same pattern as `extract_metadata.py` which imports it locally). + +- **Acceptance gate**: Trigger "Generate Missing Canonical Scenes" twice in quick succession — worker logs show `"already in-flight — skipping duplicate"` for the second batch; no file ends up being tessellated twice. - **Dependencies**: none -- **Risk**: Low — each migration is additive (ADD VALUE, ADD COLUMN, UPDATE). Check for phantom drops before running. +- **Risk**: Low — same pattern as `process_step_file`, TTL 30min covers worst-case tessellation time. -### [ ] Task 2: TypeScript check -- **What**: Run `docker compose exec frontend npx tsc --noEmit` to verify no type errors in the frontend changes -- **Acceptance gate**: Zero TypeScript errors -- **Dependencies**: none (frontend hot-reload, no rebuild needed) -- **Risk**: Low +### [x] Task 2: Add Redis dedup lock to `generate_usd_master_task` -### [ ] Task 3: Rebuild and restart backend + render-worker -- **What**: `docker compose up -d --build backend worker render-worker beat` — picks up new Dockerfile (usd-core), new tasks, and new migrations -- **Acceptance gate**: `docker compose logs backend | grep "Application startup complete"` and `docker compose exec render-worker python3 -c "from pxr import Usd; print(Usd.GetVersion())"` both succeed +- **File**: `backend/app/domains/pipeline/tasks/export_glb.py` +- **What**: Same pattern at the top of `generate_usd_master_task`, after `pl.step_start(...)`: + + ```python + import redis as _redis_lib + _lock_key = f"usd_master_lock:{cad_file_id}" + _r = _redis_lib.from_url(app_settings.redis_url) + _acquired = _r.set(_lock_key, "1", nx=True, ex=1800) # 30-min TTL + if not _acquired: + logger.warning("generate_usd_master_task: %s already in-flight — skipping duplicate", cad_file_id) + pl.step_done("usd_master", result={"skipped": True, "reason": "duplicate"}) + return {"skipped": True} + ``` + + Wrap the rest of the function body in `try: ... finally: _r.delete(_lock_key)`. + +- **Acceptance gate**: Trigger "Generate Missing USD Masters" while GLB tasks are still running — worker logs show USD tasks skipping duplicates instead of starting a second tessellation. - **Dependencies**: Task 1 -- **Risk**: Medium — `usd-core` pip install adds build time; if it fails the render-worker won't start - -### [ ] Task 4: Commit all P2 work -- **What**: Stage and commit all uncommitted P2 files in a single `feat(P2)` commit -- **Acceptance gate**: `git status` shows clean working tree (except LEARNINGS.md and review-report.md which can be included) -- **Dependencies**: Tasks 1–3 (verify before committing) - **Risk**: Low -### [ ] Task 5: Smoke-test end-to-end via Admin panel -- **What**: Via Admin → "Generate Missing Canonical Scenes" to regenerate GLBs with `partKeyMap` + auto-chain USD masters for existing CAD files -- **Acceptance gate**: - - `GET /api/cad/{id}/scene-manifest` returns `{"parts": [...], ...}` for a processed CadFile - - ThreeDViewer loads, click a part → MaterialPanel shows assignment provenance - - Assign a material → reload page → assignment still present -- **Dependencies**: Task 3 -- **Risk**: Medium — existing CAD files need backfill; may take minutes for bulk jobs to complete +### [x] Task 3: Fix `eng.dispose()` placement in cache-hit early-return path + +- **File**: `backend/app/domains/pipeline/tasks/export_glb.py` +- **What**: In `generate_gltf_geometry_task`, the cache-hit path (lines 86–95) calls `eng.dispose()` at line 89 while still inside the `with Session(eng)` block, then returns. Move `eng.dispose()` to *after* the `with` block exits. + + Current (broken): + ```python + with Session(eng) as session: + ... + if existing_geo: + pl.step_done(...) + eng.dispose() # ← inside with block + try: + generate_usd_master_task.delay(cad_file_id) + ... + return {"cached": True, ...} + eng.dispose() # normal path + ``` + + Fixed: remove the `eng.dispose()` at line 89, and move the `generate_usd_master_task.delay()` + `return` to after the `with` block: + + ```python + _cache_hit_asset_id: str | None = None + with Session(eng) as session: + ... + if existing_geo: + logger.info("[CACHE] hash match — skipping geometry GLB tessellation for %s", cad_file_id) + pl.step_done("export_glb_geometry", result={"cached": True, "asset_id": str(existing_geo.id)}) + _cache_hit_asset_id = str(existing_geo.id) + eng.dispose() + + if _cache_hit_asset_id is not None: + try: + generate_usd_master_task.delay(cad_file_id) + except Exception: + logger.debug("Could not queue generate_usd_master_task from cache-hit path (non-fatal)") + return {"cached": True, "asset_id": _cache_hit_asset_id} + + # ... rest of function (tessellation path) + ``` + +- **Acceptance gate**: `docker compose exec render-worker python3 -c "import app"` (no import errors); cache-hit path still skips tessellation and chains USD master. +- **Dependencies**: none +- **Risk**: Low — pure refactor, no logic change. + +### [x] Task 4: Add `updated_at` in `save_manual_material_overrides` + +- **File**: `backend/app/api/routers/cad.py` +- **What**: In `save_manual_material_overrides` (around line 537), add `cad.updated_at = datetime.utcnow()` before `await db.commit()`: + + ```python + cad.manual_material_overrides = body.overrides + cad.updated_at = datetime.utcnow() # ← add this line + await db.commit() + ``` + +- **Acceptance gate**: `PUT /api/cad/{id}/manual-material-overrides` → `GET /api/cad/{id}` shows updated `updated_at` timestamp. +- **Dependencies**: none +- **Risk**: None ## Migration Check -Three migrations are pending in the working tree: -- `060_usd_master_asset_type.py` — additive enum value -- `061_material_assignment_layers.py` — additive JSONB columns -- `062_rename_tessellation_settings.py` — UPDATE on `system_settings` rows (already checked: migration 062 was applied per review-report) - -**Before running**: read each migration file to confirm no unexpected DROP statements. +No migration required — no new columns or tables. ## Order Recommendation -Migrations → TypeScript check → Rebuild → Commit → Smoke test +Tasks 3 and 4 are independent cleanup items — implement first (low risk). +Tasks 1 and 2 are the core dedup fix — implement after. + +Order: Task 4 → Task 3 → Task 1 → Task 2 ## Risks / Open Questions -- `usd-core` build in Docker may be slow (first build) — expected, not a problem -- Migration 062 may already be applied (review noted "verified by 0-row SELECT") — `alembic upgrade head` is idempotent if so -- Existing CAD files need backfill for `partKeyMap` in GLB extras — handled by "Generate Missing Canonical Scenes" bulk action -- `resolvePartKey()` falls back to identity (raw mesh name) for GLBs generated before this change — graceful degradation, not a blocking issue +- Redis TTL of 30 minutes: if a task crashes hard (OOM, SIGKILL) without running `finally`, the lock stays for 30 minutes. This is the same tradeoff as `process_step_file`. Acceptable. +- `generate_usd_master_task` is also queued by the cache-hit path in `generate_gltf_geometry_task` — that chained call will be deduplicated by the lock too if the primary USD task is already running. Correct behaviour. +- The auto-chain from `generate_gltf_geometry_task → generate_usd_master_task` is still desirable (keeps canonical scene up-to-date after a fresh GLB). The lock prevents the *duplicate*, not the *legitimate* chain. diff --git a/review-report.md b/review-report.md index 6a69820..410057d 100644 --- a/review-report.md +++ b/review-report.md @@ -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 (`/// `) 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.