Browse Source

feat(slicer): log sidecar reject reason on bundle import failure (#1312)

  The route mapped sidecar 4xx/5xx to HTTPException with detail but never
  logged it. Reporters seeing a 400 toast were giving us only the status
  code, not the reason, and the access-log line was all that landed in
  support bundles.

  Add WARNING-level logs on each error branch (400 SlicerInputError,
  503 SlicerApiUnavailableError, 502 SlicerApiError) with the sidecar's
  own message + the filename / byte count / configured URL. Next reporter
  on this code path produces a support bundle that contains the answer.
maziggy 2 weeks ago
parent
commit
9cde2e37fd

File diff suppressed because it is too large
+ 0 - 0
CHANGELOG.md


+ 15 - 1
backend/app/api/routes/slicer_presets.py

@@ -427,11 +427,25 @@ async def import_slicer_bundle(
     except SlicerInputError as e:
         # Sidecar's 4xx — most likely a non-.bbscfg upload, a corrupt zip,
         # or a path-traversal entry that the manifest validator caught.
-        # Surface verbatim so the user sees the actual reason in the toast.
+        # Log the detail so it lands in the support bundle: the FE-only
+        # toast was leaving us blind during triage (#1312).
+        logger.warning(
+            "Bundle import rejected by sidecar (%s, %d bytes): %s",
+            filename,
+            len(contents),
+            e,
+        )
         raise HTTPException(status_code=400, detail=str(e)) from e
     except SlicerApiUnavailableError as e:
+        logger.warning("Bundle import: sidecar unreachable (%s): %s", api_url, e)
         raise HTTPException(status_code=503, detail=str(e)) from e
     except SlicerApiError as e:
+        logger.warning(
+            "Bundle import: sidecar server error (%s, %d bytes): %s",
+            filename,
+            len(contents),
+            e,
+        )
         # 5xx from the sidecar's import path is rare — usually a disk
         # write failure inside DATA_PATH/bundles. 502 (bad gateway) is
         # closer to the truth than 500 here, since we're proxying.

+ 7 - 1
backend/tests/unit/test_slicer_presets.py

@@ -516,7 +516,7 @@ class TestBundleRoutes:
         assert exc.value.status_code == 400
 
     @pytest.mark.asyncio
-    async def test_import_bundle_sidecar_400_passes_through(self):
+    async def test_import_bundle_sidecar_400_passes_through(self, caplog):
         from io import BytesIO
 
         from fastapi import HTTPException, UploadFile
@@ -527,6 +527,7 @@ class TestBundleRoutes:
         with (
             patch.object(sp, "_resolve_slicer_api_url", AsyncMock(return_value="http://ok")),
             patch.object(sp, "SlicerApiService", return_value=svc),
+            caplog.at_level("WARNING", logger="backend.app.api.routes.slicer_presets"),
             pytest.raises(HTTPException) as exc,
         ):
             await sp.import_slicer_bundle(
@@ -535,6 +536,11 @@ class TestBundleRoutes:
                 _=None,
             )
         assert exc.value.status_code == 400
+        # #1312: the sidecar's reject reason MUST land in the log so it
+        # ends up in support bundles without us having to ask reporters
+        # to copy the FE toast.
+        assert any("bad zip" in r.message for r in caplog.records)
+        assert any("x.bbscfg" in r.message for r in caplog.records)
 
     @pytest.mark.asyncio
     async def test_import_bundle_sidecar_unreachable_returns_503(self):

Some files were not shown because too many files changed in this diff