ソースを参照

Explain an oversized model instead of reporting a slicer crash (#2802)

The sidecar caps model uploads and reports a rejection as a bare
HTTP 500 "File too large" -- multer's MulterError is not the sidecar's
AppError, so its handler falls through to the default status. A 500
reads as a crash inside the slicer, and the one message Bambuddy had
about request size was written for the 413 a reverse proxy sends, so
it never appeared. The reporter tried MAX_FILE_SIZE, BODY_PARSER_LIMIT
and EXPRESS_PAYLOAD_LIMIT, stopped nginx, and moved from Windows to
Docker -- none of which the sidecar reads.

Match the rejection by what it says rather than by its status, so an
installation still on an older sidecar image gets the same explanation.
The 500 match is strict -- the body must be only multer's message --
because a genuine CLI failure is also a 500 and has to keep reaching
the embedded-settings fallback. Old images are told to update, since
they have no setting to change; current ones are told which one to set.

Raising SlicerInputError rather than SlicerApiServerError is also what
skips the fallback retry, which had been re-uploading the identical
oversized file after a second 25-second 3MF conversion.

Log the model size on every slice. Nothing recorded it, so a support
package from a slice that died on an upload cap looked exactly like one
that died on a bad profile, and this had to be sized by hand.

Fall back to the exception class name when a transport error stringifies
empty -- three lines of the reporter's log read "Slicer sidecar
unreachable: " and stopped there.

Needs a sidecar image update to take full effect; MAX_MODEL_UPLOAD_MB is
documented in slicer-api/.env.example.
maziggy 3 週間 前
コミット
a849759469

ファイルの差分が大きいため隠しています
+ 0 - 0
CHANGELOG.md


+ 111 - 7
backend/app/services/slicer_api.py

@@ -132,7 +132,84 @@ def _format_sidecar_error(response: httpx.Response) -> str:
     return (message or details or response.text)[:500]
 
 
-def _handle_slice_response(response: httpx.Response, *, export_3mf: bool) -> SliceResult:
+def _transport_error_reason(exc: httpx.RequestError) -> str:
+    """Describe a transport failure, even when the exception carries no message.
+
+    Several ``httpx.RequestError`` subclasses are raised with no args, so
+    ``str(exc)`` is the empty string — which is how three lines of the #2802
+    reporter's support package came to read ``Slicer sidecar unreachable:``
+    with nothing after the colon. The class name is not much, but it
+    distinguishes a refused connection from a protocol error, and a log line
+    that names nothing is worth less than one that names the exception type.
+    """
+    return str(exc) or type(exc).__name__
+
+
+# How the sidecar says "your model is bigger than my cap", across versions.
+# Images built before the cap became configurable answer with multer's raw
+# ``LIMIT_FILE_SIZE`` text under a **500** — ``MulterError`` is not the
+# sidecar's ``AppError``, so its handler falls through to the default status —
+# while current ones send a 413 naming the limit and the env var that raises
+# it. Matching on text rather than status covers both, and matters because a
+# 500 otherwise reads as a slicer crash and sends people off tuning reverse
+# proxies that were never in the path (#2802).
+#
+# Deliberately specific: a proxy's own "413 Request Entity Too Large" must NOT
+# match, because that one really is fixed at the proxy and gets its own advice.
+_UPLOAD_TOO_LARGE_MARKERS = (
+    "file too large",
+    "upload limit",
+    "max_model_upload_mb",
+)
+
+# A sidecar that says which knob raises the cap is new enough to have one.
+# Older ones only ever emit multer's bare "File too large", and for those the
+# advice has to be "update the image" — there is no env var to set.
+_CONFIGURABLE_CAP_MARKERS = ("upload limit", "max_model_upload_mb")
+
+
+def _upload_size_rejection(response: httpx.Response, model_size_bytes: int | None) -> str | None:
+    """Return an explanation if the sidecar refused the upload as oversized.
+
+    The 500 case is matched strictly — the body has to be *only* multer's
+    message — because a 500 is also how a genuine CLI failure arrives, and
+    those must keep reaching the embedded-settings fallback. A CLI failure
+    always carries the slicer's stderr in ``details``, so it never reduces to
+    the bare string on its own.
+    """
+    detail = _format_sidecar_error(response)
+    lowered = detail.lower()
+    if response.status_code >= 500:
+        if lowered.strip() != "file too large":
+            return None
+    elif not any(marker in lowered for marker in _UPLOAD_TOO_LARGE_MARKERS):
+        return None
+
+    size = f"{model_size_bytes / (1024 * 1024):.0f} MB " if model_size_bytes else ""
+    # Shared preamble: both variants must rule out the layers people reach for
+    # first, because those are the ones that look like they should apply.
+    common = (
+        f"The slicer sidecar refused the {size}model file as too large. The limit lives inside "
+        "the sidecar container, so it is neither a Bambuddy setting nor a reverse-proxy one — "
+        "raising 'client_max_body_size' or a proxy body limit will not change it."
+    )
+
+    if any(marker in lowered for marker in _CONFIGURABLE_CAP_MARKERS):
+        return (
+            f"{common} Raise it by setting MAX_MODEL_UPLOAD_MB on the slicer-api service and "
+            f"restarting it. Sidecar said: {detail}"
+        )
+    return (
+        f"{common} This sidecar image predates the configurable cap and is fixed at 100 MB — "
+        "update it with 'cd slicer-api/ && docker compose pull && docker compose up -d', which "
+        "raises the default and adds MAX_MODEL_UPLOAD_MB for going higher still. "
+        f"Sidecar said: {detail}"
+    )
+
+
+def _handle_slice_response(
+    response: httpx.Response, *, export_3mf: bool, model_size_bytes: int | None = None
+) -> SliceResult:
     """Turn a sidecar ``/slice`` HTTP response into a validated ``SliceResult``.
 
     Shared by ``slice_with_profiles`` / ``slice_without_profiles`` so the status
@@ -151,6 +228,14 @@ def _handle_slice_response(response: httpx.Response, *, export_3mf: bool) -> Sli
         SlicerInputError: 4xx from the sidecar (bad input / proxy body limit).
         SlicerApiServerError: 5xx, or a 2xx whose body is not a valid 3MF.
     """
+    # Checked ahead of the status branches because the same rejection arrives
+    # as a 500 from older sidecars and a 413 from newer ones, and because
+    # raising SlicerInputError (rather than SlicerApiServerError) is what stops
+    # the library route retrying the identical oversized upload with embedded
+    # settings — a second 25-second conversion for a guaranteed same answer.
+    oversized = _upload_size_rejection(response, model_size_bytes)
+    if oversized:
+        raise SlicerInputError(oversized)
     if response.status_code == 413:
         # A 413 almost never comes from the slicer itself — it's a reverse proxy
         # (nginx/SWAG/Traefik) or a CDN capping the multipart upload (model +
@@ -317,7 +402,7 @@ class SlicerApiService:
         try:
             response = await self._client.get(f"{self.base_url}/health", timeout=10.0)
         except httpx.RequestError as exc:
-            raise SlicerApiUnavailableError(f"Slicer sidecar unreachable: {exc}") from exc
+            raise SlicerApiUnavailableError(f"Slicer sidecar unreachable: {_transport_error_reason(exc)}") from exc
         if response.status_code >= 400:
             raise SlicerApiUnavailableError(f"Slicer sidecar /health returned {response.status_code}")
         return response.json()
@@ -358,7 +443,7 @@ class SlicerApiService:
                 timeout=15.0,
             )
         except httpx.RequestError as exc:
-            raise SlicerApiUnavailableError(f"Slicer sidecar unreachable: {exc}") from exc
+            raise SlicerApiUnavailableError(f"Slicer sidecar unreachable: {_transport_error_reason(exc)}") from exc
 
         if response.status_code == 404:
             # Sidecar predates the endpoint. Not an error, and specifically not
@@ -397,7 +482,7 @@ class SlicerApiService:
         try:
             response = await self._client.get(f"{self.base_url}/profiles/bundled", timeout=10.0)
         except httpx.RequestError as exc:
-            raise SlicerApiUnavailableError(f"Slicer sidecar unreachable: {exc}") from exc
+            raise SlicerApiUnavailableError(f"Slicer sidecar unreachable: {_transport_error_reason(exc)}") from exc
         if response.status_code >= 400:
             raise SlicerApiUnavailableError(f"Slicer sidecar /profiles/bundled returned {response.status_code}")
         return response.json()
@@ -530,7 +615,7 @@ class SlicerApiService:
         try:
             return post_task.result()
         except httpx.RequestError as exc:
-            raise SlicerApiUnavailableError(f"Slicer sidecar unreachable: {exc}") from exc
+            raise SlicerApiUnavailableError(f"Slicer sidecar unreachable: {_transport_error_reason(exc)}") from exc
 
     async def slice_with_profiles(
         self,
@@ -612,8 +697,9 @@ class SlicerApiService:
         # and surfaces structured updates via on_progress. Uses a
         # short-tick poll (1s) since the slicer emits stage changes
         # several times per minute on complex models.
+        _log_slice_request(model_filename, model_bytes, plate=plate, profiles=len(filament_profile_jsons) + 2)
         response = await self._post_slice(files=files, data=data, request_id=request_id, on_progress=on_progress)
-        return _handle_slice_response(response, export_3mf=export_3mf)
+        return _handle_slice_response(response, export_3mf=export_3mf, model_size_bytes=len(model_bytes))
 
     async def slice_without_profiles(
         self,
@@ -668,8 +754,26 @@ class SlicerApiService:
         # embedded-settings fallback path triggered by an Orca/Bambu CLI
         # segfault on complex H2D models — both want to keep updating
         # the user's toast through the slow operation.
+        _log_slice_request(model_filename, model_bytes, plate=plate, profiles=0)
         response = await self._post_slice(files=files, data=data, request_id=request_id, on_progress=on_progress)
-        return _handle_slice_response(response, export_3mf=export_3mf)
+        return _handle_slice_response(response, export_3mf=export_3mf, model_size_bytes=len(model_bytes))
+
+
+def _log_slice_request(filename: str, model_bytes: bytes, *, plate: int | None, profiles: int) -> None:
+    """Record what is being sent to the sidecar, size included.
+
+    Nothing used to log the payload size, so a support package from a slice
+    that failed on an upload cap looked identical to one that failed on a bad
+    profile — #2802 had to be sized by probing a sidecar by hand. One line per
+    slice is cheap next to the operation it describes.
+    """
+    logger.info(
+        "Slicing %s (%.1f MB) plate=%s with %d profile(s)",
+        filename,
+        len(model_bytes) / (1024 * 1024),
+        "all" if plate is None else plate,
+        profiles,
+    )
 
 
 def _add_layout_flags(data: dict[str, str], *, arrange: bool, orient: bool) -> None:

+ 245 - 0
backend/tests/unit/test_slicer_upload_size_rejection.py

@@ -0,0 +1,245 @@
+"""The sidecar's upload cap, reported as something the user can act on (#2802).
+
+The slicer sidecar bounds the size of the model it will accept. multer raises
+that rejection as a ``MulterError``, which is not the sidecar's ``AppError`` —
+so on every image built before the cap became configurable, the sidecar's error
+handler fell through to its default status and answered:
+
+    HTTP 500 {"message": "File too large"}
+
+A 500 reads as "the slicer crashed". Bambuddy's one good message about request
+size lived behind ``if response.status_code == 413``, so it never fired, and the
+reporter of #2802 spent an evening setting ``MAX_FILE_SIZE``,
+``BODY_PARSER_LIMIT`` and ``EXPRESS_PAYLOAD_LIMIT`` and stopping nginx — none of
+which the sidecar reads, on a proxy that was never in the path.
+
+Two things follow, and both are pinned here:
+
+- The rejection is recognised by its *text*, not its status, so it is handled
+  the same whether the sidecar is old (500) or current (413).
+- It raises ``SlicerInputError`` rather than ``SlicerApiServerError``. That is
+  what stops ``POST /library/files/{id}/slice`` retrying the identical
+  oversized upload "with embedded settings" — a second 25-second 3MF
+  conversion for a guaranteed-identical answer, which the reporter's log shows
+  happening on every attempt.
+"""
+
+import httpx
+import pytest
+
+from backend.app.services.slicer_api import (
+    SlicerApiServerError,
+    SlicerApiService,
+    SlicerApiUnavailableError,
+    SlicerInputError,
+    _transport_error_reason,
+)
+
+SLICE_ARGS = {
+    "model_bytes": b"x" * (3 * 1024 * 1024),
+    "model_filename": "0399 Bidoof.3mf",
+    "printer_profile_json": "{}",
+    "process_profile_json": "{}",
+    "filament_profile_jsons": ["{}"],
+}
+
+
+def _service(handler) -> SlicerApiService:
+    client = httpx.AsyncClient(transport=httpx.MockTransport(handler))
+    return SlicerApiService("http://sidecar:3001", client=client)
+
+
+def _responder(status_code: int, payload: dict):
+    def handler(request: httpx.Request) -> httpx.Response:
+        return httpx.Response(status_code, json=payload)
+
+    return handler
+
+
+class TestOversizeUploadIsRecognised:
+    @pytest.mark.asyncio
+    async def test_a_500_file_too_large_is_treated_as_bad_input(self):
+        """The exact shape an un-updated sidecar returns."""
+        svc = _service(_responder(500, {"message": "File too large"}))
+
+        with pytest.raises(SlicerInputError) as excinfo:
+            await svc.slice_with_profiles(**SLICE_ARGS)
+
+        assert "too large" in str(excinfo.value)
+
+    @pytest.mark.asyncio
+    async def test_a_413_from_a_current_sidecar_is_handled_the_same(self):
+        """Once the sidecar maps MulterError properly it sends 413 instead."""
+        svc = _service(
+            _responder(
+                413,
+                {
+                    "message": "The model file exceeds this slicer's 512 MB upload limit.",
+                    "details": "Raise it by setting MAX_MODEL_UPLOAD_MB.",
+                },
+            )
+        )
+
+        with pytest.raises(SlicerInputError):
+            await svc.slice_with_profiles(**SLICE_ARGS)
+
+    @pytest.mark.asyncio
+    async def test_it_is_not_a_server_error(self):
+        """The distinction the retry logic in library.py branches on.
+
+        ``SlicerApiServerError`` is the "the CLI fell over, try the other
+        request shape" signal. An upload the sidecar never accepted is not
+        that, and retrying it uploads the same too-big file again.
+        """
+        svc = _service(_responder(500, {"message": "File too large"}))
+
+        with pytest.raises(SlicerInputError):
+            await svc.slice_with_profiles(**SLICE_ARGS)
+
+        # Belt and braces: SlicerInputError must not be a subclass of the type
+        # the fallback catches, or the branch above is decorative.
+        assert not issubclass(SlicerInputError, SlicerApiServerError)
+
+
+class TestTheMessageIsActionable:
+    @pytest.mark.asyncio
+    async def test_it_names_the_model_size(self):
+        """Support packages carried no size at all; #2802 had to be probed."""
+        svc = _service(_responder(500, {"message": "File too large"}))
+
+        with pytest.raises(SlicerInputError) as excinfo:
+            await svc.slice_with_profiles(**SLICE_ARGS)
+
+        assert "3 MB" in str(excinfo.value)
+
+    @pytest.mark.asyncio
+    async def test_it_rules_out_the_layers_the_reporter_tried(self):
+        """Naming the wrong knobs is the point: they were tried first."""
+        svc = _service(_responder(500, {"message": "File too large"}))
+
+        with pytest.raises(SlicerInputError) as excinfo:
+            await svc.slice_with_profiles(**SLICE_ARGS)
+
+        message = str(excinfo.value)
+        assert "reverse-proxy" in message
+        assert "client_max_body_size" in message
+
+    @pytest.mark.asyncio
+    async def test_an_old_sidecar_is_told_to_update_not_to_set_a_variable(self):
+        """There is no env var to set on an image that predates the cap.
+
+        Telling that user to set MAX_MODEL_UPLOAD_MB would send them round the
+        loop the reporter already did: change a setting, restart, no effect.
+        """
+        svc = _service(_responder(500, {"message": "File too large"}))
+
+        with pytest.raises(SlicerInputError) as excinfo:
+            await svc.slice_with_profiles(**SLICE_ARGS)
+
+        message = str(excinfo.value)
+        assert "docker compose pull" in message
+        assert "100 MB" in message
+
+    @pytest.mark.asyncio
+    async def test_a_current_sidecar_is_told_which_variable_to_set(self):
+        """Once the image is current, the fix is one env var, not another pull."""
+        svc = _service(
+            _responder(
+                413,
+                {"message": "The model file exceeds this slicer's 512 MB upload limit."},
+            )
+        )
+
+        with pytest.raises(SlicerInputError) as excinfo:
+            await svc.slice_with_profiles(**SLICE_ARGS)
+
+        message = str(excinfo.value)
+        assert "MAX_MODEL_UPLOAD_MB" in message
+        assert "docker compose pull" not in message
+
+    @pytest.mark.asyncio
+    async def test_it_keeps_what_the_sidecar_said(self):
+        """Never swallow the upstream text — it identifies the sidecar version."""
+        svc = _service(_responder(413, {"message": "The model file exceeds this slicer's 256 MB upload limit."}))
+
+        with pytest.raises(SlicerInputError) as excinfo:
+            await svc.slice_with_profiles(**SLICE_ARGS)
+
+        assert "256 MB" in str(excinfo.value)
+
+
+class TestOtherFailuresAreUnaffected:
+    @pytest.mark.asyncio
+    async def test_an_ordinary_cli_failure_is_still_a_server_error(self):
+        """The embedded-settings fallback must keep working for real crashes."""
+        svc = _service(
+            _responder(
+                500,
+                {
+                    "message": "Slicing failed with error from slicer",
+                    "details": "Slicer process failed (exit code 250)",
+                },
+            )
+        )
+
+        with pytest.raises(SlicerApiServerError):
+            await svc.slice_with_profiles(**SLICE_ARGS)
+
+    @pytest.mark.asyncio
+    async def test_a_cli_error_that_merely_mentions_a_large_file_is_not_hijacked(self):
+        """A 500 only counts as an upload rejection if that is all it says.
+
+        The slicer's own diagnostics land in ``details``, and treating one of
+        those as a size rejection would rob it of the embedded-settings retry
+        that exists to recover from CLI failures.
+        """
+        svc = _service(
+            _responder(
+                500,
+                {
+                    "message": "Slicing failed with error from slicer",
+                    "details": "stderr: output file too large to write",
+                },
+            )
+        )
+
+        with pytest.raises(SlicerApiServerError):
+            await svc.slice_with_profiles(**SLICE_ARGS)
+
+    @pytest.mark.asyncio
+    async def test_a_proxy_413_still_names_the_proxy(self):
+        """A 413 that is *not* the sidecar's own cap is a proxy body limit.
+
+        Those really are fixed with ``client_max_body_size``, so that advice
+        has to survive — the new branch must not swallow every 413.
+        """
+        svc = _service(_responder(413, {"message": "<html>413 Request Entity Too Large</html>"}))
+
+        with pytest.raises(SlicerInputError) as excinfo:
+            await svc.slice_with_profiles(**SLICE_ARGS)
+
+        assert "client_max_body_size" in str(excinfo.value)
+
+
+class TestTransportErrorsAlwaysNameSomething:
+    """Three lines of the #2802 support package read "unreachable: " and stop."""
+
+    def test_an_exception_with_no_message_falls_back_to_its_type(self):
+        assert _transport_error_reason(httpx.ConnectError("")) == "ConnectError"
+
+    def test_a_real_message_is_preferred(self):
+        assert _transport_error_reason(httpx.ConnectError("All connection attempts failed")) == (
+            "All connection attempts failed"
+        )
+
+    @pytest.mark.asyncio
+    async def test_the_slice_path_never_reports_an_empty_reason(self):
+        def handler(request: httpx.Request) -> httpx.Response:
+            raise httpx.ReadError("")
+
+        svc = _service(handler)
+
+        with pytest.raises(SlicerApiUnavailableError) as excinfo:
+            await svc.slice_with_profiles(**SLICE_ARGS)
+
+        assert str(excinfo.value).strip().endswith("ReadError")

+ 6 - 0
slicer-api/.env.example

@@ -12,3 +12,9 @@ BAMBU_API_PORT=3001
 #                    (e.g. SIDECAR_TAG=bambuddy-0.2.5)
 #   daily            track the bambuddy:daily channel
 SIDECAR_TAG=latest
+
+# Largest model file the sidecars will accept for a slice, in megabytes.
+# Raise it if a big multi-colour project is rejected as "too large" -- the
+# cap is enforced inside the sidecar, so no reverse-proxy body limit affects
+# it. Leave it unset for the 512 MB default.
+#MAX_MODEL_UPLOAD_MB=512

+ 4 - 0
slicer-api/docker-compose.yml

@@ -37,6 +37,8 @@ services:
     environment:
       NODE_ENV: production
       PORT: "3000"
+      # Largest model accepted for a slice, in MB. See .env.example.
+      MAX_MODEL_UPLOAD_MB: "${MAX_MODEL_UPLOAD_MB:-512}"
     healthcheck:
       test: ["CMD", "curl", "-f", "http://localhost:3000/health"]
       interval: 30s
@@ -55,6 +57,8 @@ services:
     environment:
       NODE_ENV: production
       PORT: "3000"
+      # Largest model accepted for a slice, in MB. See .env.example.
+      MAX_MODEL_UPLOAD_MB: "${MAX_MODEL_UPLOAD_MB:-512}"
     healthcheck:
       test: ["CMD", "curl", "-f", "http://localhost:3000/health"]
       interval: 30s

この差分においてかなりの量のファイルが変更されているため、一部のファイルを表示していません