diff --git a/backend/app/services/slicer_api.py b/backend/app/services/slicer_api.py index 65cc0200b..4317f0d0a 100644 --- a/backend/app/services/slicer_api.py +++ b/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: diff --git a/backend/tests/unit/test_slicer_upload_size_rejection.py b/backend/tests/unit/test_slicer_upload_size_rejection.py new file mode 100644 index 000000000..005f31806 --- /dev/null +++ b/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": "413 Request Entity Too Large"})) + + 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") diff --git a/slicer-api/.env.example b/slicer-api/.env.example index ac001574e..42b146a14 100644 --- a/slicer-api/.env.example +++ b/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 diff --git a/slicer-api/docker-compose.yml b/slicer-api/docker-compose.yml index 9a3709675..35bdbd005 100644 --- a/slicer-api/docker-compose.yml +++ b/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