From f3b1c5916935ddd8b69782bc82501aeb9e47948c Mon Sep 17 00:00:00 2001 From: maziggy Date: Mon, 7 Sep 2026 10:39:10 +0200 Subject: [PATCH] fix(orca-cloud): close the HTTP client when an authenticated build fails OrcaCloudService owns an httpx client from construction, and every path in _build_authenticated_service after that point can raise: no stored refresh token, a rejected refresh, an unreachable Orca, and the token-rotation write. On success the caller closes the client. On failure nobody is ever handed it, so all four paths leaked one into the connection pool. That went unnoticed while the only callers were routes, where the trigger is a person retrying a broken sign-in a handful of times. It stopped being harmless in 9434875f, which added a caller in spool assignment -- one build per Orca-referenced spool, failing on every assignment for as long as the stored credentials cannot be refreshed. The unwind guard catches BaseException rather than Exception: a cancelled request leaks the client just as surely as a failed refresh, and cancellation during shutdown is exactly when dangling sockets are least welcome. The close inside it is guarded in turn, so a failing cleanup cannot replace the error the caller needs to see -- least of all a CancelledError, which has to keep propagating for cancellation to work at all. Six tests. Four fail against the unguarded builder, verified by reverting the guard and re-running; the other two pin the surrounding contract (a failing close must not mask the real error, and a successful build must leave the client open for its caller) and pass either way. The shared _expired_service helper now gives the mock an awaitable close(), so the four pre-existing refresh tests exercise the same path. Also corrects two comments and the changelog entry from 9434875f, which overstated what the captures support. They claimed Bambu Cloud returns a preset's filament_id in either of two places and only one was read. The responses recorded in #1053 show something narrower: a Studio-created preset carries it on the envelope, and an Orca-created one has none at all -- the envelope says null and `setting` is a delta from the base. The `setting` lookup stays as belt-and-braces for a shape no captured response has needed yet, but it is not why a custom profile reached the slicer as its base. That is the OrcaSlicer preset format having no filament_id field, filed upstream as OrcaSlicer PR #13315. The eight-character truncation is now evidenced across three models rather than one -- an A1 storing PFUS9DDC of PFUS9DDC938FE3AB8F, a P1S storing PFUS7A65 of PFUS7A65290D3DADC4, and an H2D storing 8219C45D of an Orca profile UUID. --- CHANGELOG.md | 2 +- backend/app/api/routes/orca_cloud.py | 62 +++++++---- .../app/services/slicer_filament_resolver.py | 21 +++- backend/tests/unit/test_orca_cloud_refresh.py | 105 ++++++++++++++++++ .../src/components/ConfigureAmsSlotModal.tsx | 8 +- 5 files changed, 165 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ff32a5ad8..ceff08a3b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,7 +28,7 @@ All notable changes to Bambuddy will be documented in this file. - **The Windows installer build is split in two so a signing request can wait for a human (SignPath Foundation)** — Release tags are Authenticode-signed through the SignPath Foundation OSS programme, and the production certificate does not sign on demand the way the self-signed test certificate does: every request has to be approved by hand in the SignPath UI, because the Foundation verifies what is being signed and which build it came from. The submitting action waits for that approval with a default timeout of 600 seconds, which is ample when the test policy approves automatically in seconds and far too short once the wait is a person noticing a tag went out. A tag pushed at night would have failed the run ten minutes later with the installer already compiled and thrown away. The compile now ends in its own job that uploads the unsigned artifact and stops; a second job downloads it, signs it, and does the release-facing work, with the wait raised to an hour. Because the artifact is uploaded before the wait begins and is addressed by id, a missed approval window is recovered by re-running the second job alone rather than rebuilding the installer — which is the reason to separate them rather than simply raise the timeout in place. The second job runs for unsigned builds too, so the daily prereleases that are deliberately left unsigned to preserve the signing quota keep going out through exactly one set of alias, artifact and release steps. The property that matters is unchanged and now recorded next to the steps that depend on it: none of the alias, upload or release-attach steps carry `always()`, so GitHub skips all three when signing fails or times out, and an unsigned `.exe` cannot reach a release. Nothing about the signed output changes, and the restructure behaves identically under the test policy — the request simply completes immediately instead of waiting — so it can be proven green before the production certificate arrives. ### Fixed -- **Custom filament profiles arrived in the slicer as Generic, or as the Bambu profile they were built on (#3003, reported by @marivo)** — a custom profile reaches an AMS slot as itself through exactly one field, the slot's filament id, and every source Bambuddy can get that id from was reading it from the wrong place or not at all. Bambu Cloud reports it in either of two spots depending on the preset and only one was read, so presets of the other shape fell back to the id of the Bambu filament they inherit from — which is what the slicer then showed. Orca Cloud profiles were never looked up at all: their ids are UUIDs the printer cannot store, so the code went straight to a generic without ever asking whether the profile had a usable id of its own, which it normally does. And the Configure AMS Slot dialog, when it could not find a real id, sent the preset's own identifier instead — that field holds eight characters, less than half an identifier, so it was stored truncated and acknowledged as a success, leaving the slot pointing at nothing and the printer's calibration table, keyed by the same field, without a slot to key. All four sources are now tried in order — Orca Cloud, Bambu Cloud, imported local profiles, then a generic for the material — and nothing the printer cannot store is sent. A profile that genuinely has no filament id of its own still cannot be told apart from the one it inherits from; there is nowhere else in what a printer publishes to put it. +- **Custom filament profiles arrived in the slicer as Generic, or as the Bambu profile they were built on (#3003, reported by @marivo)** — the slot's filament id is the one field a custom profile travels in, and it holds eight characters on the printer. Bambuddy was putting an eighteen-character preset identifier in it whenever it could not find a real filament id. The printer stored the first eight and reported success, so the slot pointed at something that resolves nowhere: the slicer showed Generic and the printer's calibration table, keyed by the same field, no longer had a slot to key. Three printers in the support archive show it happening — an A1, a P1S and an H2D — so it was never specific to one model. Bambuddy now sends the slot's existing filament id, or the generic one for the material, both of which fit. Orca Cloud profiles were additionally never looked up at all, so theirs went in as a thirty-six-character identifier and fared worse still; they are now resolved like every other source. Also fixed on the way through: a failed Orca Cloud sign-in left its HTTP connection behind instead of closing it, which went unnoticed while only the settings page could trigger it and would have repeated on every spool assignment once the lookup above started using the same code. A profile that carries no filament id of its own — which is every profile created in OrcaSlicer, whose preset format has no such field — still cannot be told apart from the one it inherits from. - **Hovering a muted control in the light theme made its label vanish (#1909, reported by @AntonPalmqvist)** — the tab strip on the Print Queue page was where it got noticed: point at **Batches** and the word turned white on a near-white background. It was never about that tab. The light theme keeps `text-white` readable by remapping it to the theme's foreground colour, but that remapping only ever matched the plain utility, not the `hover:` variant Tailwind compiles to a different selector — so roughly 400 controls across the app that dim their label at rest and brighten it on hover were brightening it to literal white, whatever the theme. They now follow the theme like everything else. Dark themes are unaffected, their foreground colour already being white, and the handful of buttons that turn a solid accent colour on hover read better for it rather than worse. - **Every camera stopped working on 1.2.5.4 (#3001, reported by @Jieper001, confirmed by @JmanB52D and @hikingthunder)** — live view, snapshots, timelapse frames and the camera diagnostic all failed at once on every RTSP model — X1, H2 and P2 — with the in-app diagnostic reporting `capture_exception` at 0 ms while network reachability passed at 1 ms. That 0 ms is the whole story: the failure happened before a socket was opened. The RTSPS proxy added in 1.2.5.4 finished by hanging its set of in-flight connection handlers on the server object as an attribute. `asyncio.start_server` returns an `asyncio.base_events.Server`, which has a `__dict__` and accepts that; under uvloop it returns a `uvloop.loop.Server`, a Cython cdef class with no `__dict__`, which raises `AttributeError` outright. Every launch path this repo ships — the Dockerfile, `install/install.sh`, `deploy/bambuddy.service`, the Windows service and the SpoolBuddy installer — pins `--loop asyncio`, added for #1896, so none of them selects uvloop and none of them could hit this. What broke is the installs running a unit file we did not write. The Proxmox VE Helper-Scripts LXC composes its own `ExecStart` with no loop pinned, and `requirements.txt` pins `uvicorn[standard]`, which installs uvloop on Linux, so uvicorn's default `--loop auto` selects it — that is the reporter's install and the two that confirmed it. Native installs created before the #1896 pin landed on 2026-07-05 are in the same position for a different reason: `install/update.sh` never rewrites the unit file, so a service written before that date has never been given the flag by any update since. Those installs are also still exposed to #1896 itself, where a truncated Virtual Printer FTP upload corrupts a `.gcode.3mf` silently; the camera outage is simply the visible half. Hence a fix in the code rather than another flag in a unit file: the proxy now works on either loop instead of depending on the launch command to steer around it. The handler set now lives in a module-level registry keyed weakly by server, which both loops accept; keying it weakly rather than by `id(server)` means a proxy abandoned without a close takes its entry with it, instead of leaking one forever and eventually handing a new server a dead one's handlers once CPython recycles the address. A1 and P1 use the chamber-image protocol and return before the proxy is built, so they were never affected, and external RTSPS cameras caught the error and fell back to a direct connection, so they kept working without the TLS workaround. The reason the test suite could not see any of this is that `conftest` builds its event loop from the default policy, so every async test in the repo runs on the selector loop — the one loop where the assignment was legal. The regression is now pinned twice: once by a test that drives the real function on a real uvloop loop, and once by a test that gives it a `__slots__` server, so the contract holds even where uvloop is not installed. The two external-camera teardowns were also switched to `close_tls_proxy`, which #2968 introduced and left them out of, so they no longer leave handlers running past the server that owned them. - **Some archived 3MFs lost their G-code when re-imported into the File Manager (#2993, reported via the in-app form)** — they never lost it. The download serves the stored file byte for byte, and the G-code was still in the zip; what differed was who was asked. On the archive side the answer came from the file itself — the green GCODE badge reads the layer count and print time that were parsed out of the plate G-code — while the library decided from the filename alone, so a sliced 3MF stored as `Foo.3mf` rather than `Foo.gcode.3mf` carried the badge and still came back as a source-only project with no Print button. That splits on how the print reached the printer, not on anything about the file — a slicer's LAN send names it `.gcode.3mf`, while a per-plate export or a cloud-dispatched print arrives as plain `.3mf` — which is why it looked random. Both sides now ask the same question of the zip itself, and every route into the library (upload, ZIP import, MakerWorld, external-folder scan) classifies on content rather than on the name. Files already in your library are re-checked once on the next start. The backend was always willing to print these, so this was only ever the interface refusing to offer something that would have worked; a genuine model file is unaffected, and one that now shows **Print** correctly stops offering **Slice**. diff --git a/backend/app/api/routes/orca_cloud.py b/backend/app/api/routes/orca_cloud.py index b90a6cced..46ffb56ce 100644 --- a/backend/app/api/routes/orca_cloud.py +++ b/backend/app/api/routes/orca_cloud.py @@ -464,29 +464,47 @@ async def _build_authenticated_service( raise HTTPException(status_code=401, detail="Orca Cloud is not connected — sign in first.") svc = OrcaCloudService() - svc.set_tokens(creds.token, creds.refresh_token, creds.expires_at) - if not svc.is_authenticated: - if not svc.refresh_token: - raise HTTPException( - status_code=401, - detail="Orca Cloud session expired and no refresh token is stored — sign in again.", - ) + # The service owns an httpx client from construction, and every path below + # this point can raise. On success the caller closes it; on failure nobody + # ever holds it, so it has to be closed here or the connection pool leaks + # one client per failed build. That went unnoticed while the only callers + # were routes -- a person retrying a broken sign-in a few times -- and + # became worth fixing once spool assignment started building one too. + try: + svc.set_tokens(creds.token, creds.refresh_token, creds.expires_at) + if not svc.is_authenticated: + if not svc.refresh_token: + raise HTTPException( + status_code=401, + detail="Orca Cloud session expired and no refresh token is stored — sign in again.", + ) + try: + await svc.refresh() + except OrcaCloudAuthError as e: + # Refresh token was revoked or rotated out from under us. Clear + # the stale credentials so the UI flips to disconnected — unless + # the caller is a background job, which must not change sign-in + # state on its own. + if clear_on_auth_failure: + await _clear_credentials(db, user) + raise HTTPException(status_code=401, detail=f"Orca Cloud session refresh failed: {e}") from e + except OrcaCloudError as e: + raise HTTPException(status_code=502, detail=f"Orca Cloud unreachable: {e}") from e + # Persist new pair BEFORE returning. A crash between here and the + # downstream API call would still leave the user with valid stored + # tokens for the next request. + await _persist_rotated_tokens(db, user, svc.access_token, svc.refresh_token, svc.token_expiry) + except BaseException: + # BaseException, not Exception: a cancelled request leaks the client + # just as surely as a failed refresh does. The close is guarded in turn + # because failing to clean up must not replace the error the caller + # needs to see -- least of all a CancelledError, which has to keep + # propagating for cancellation to work at all. try: - await svc.refresh() - except OrcaCloudAuthError as e: - # Refresh token was revoked or rotated out from under us. Clear - # the stale credentials so the UI flips to disconnected — unless - # the caller is a background job, which must not change sign-in - # state on its own. - if clear_on_auth_failure: - await _clear_credentials(db, user) - raise HTTPException(status_code=401, detail=f"Orca Cloud session refresh failed: {e}") from e - except OrcaCloudError as e: - raise HTTPException(status_code=502, detail=f"Orca Cloud unreachable: {e}") from e - # Persist new pair BEFORE returning. A crash between here and the - # downstream API call would still leave the user with valid stored - # tokens for the next request. - await _persist_rotated_tokens(db, user, svc.access_token, svc.refresh_token, svc.token_expiry) + await svc.close() + except Exception as close_err: # noqa: BLE001 - cleanup is best-effort + logger.debug("Orca Cloud client close failed while unwinding a failed build: %s", close_err) + raise return svc diff --git a/backend/app/services/slicer_filament_resolver.py b/backend/app/services/slicer_filament_resolver.py index 254f4ba18..7c29e513b 100644 --- a/backend/app/services/slicer_filament_resolver.py +++ b/backend/app/services/slicer_filament_resolver.py @@ -231,12 +231,21 @@ async def resolve_slicer_filament( # gets it into an AMS slot as itself: the printer stores # that id, the slicer matches its presets against it, and # the 8-character field fits it exactly ("P" + 7 hex). - # Bambu Cloud puts it in either of two places -- on the - # envelope, or inside the preset JSON under `setting` -- - # the same spread `filament_type` above already handles. - # Reading only the envelope is how a custom preset fell - # through to the base_id branch below and reached the - # slicer as the Bambu profile it inherits from (#3003). + # + # Bambu Cloud normally returns it on the envelope, which is + # what the captures in #1053 show for a Studio-created + # preset (filament_id: "Pbd31b30"). The `setting` fallback + # here is belt-and-braces for a response that carries it in + # the preset JSON instead, the same spread `filament_type` + # above has to handle -- no captured response has needed it + # yet, and it costs a dict lookup to be ready for one. + # + # An Orca-created preset has no filament_id anywhere: the + # envelope says null and `setting` is a delta from the base + # (#1053 again). Those legitimately fall to base_id below + # and reach the slicer as the profile they inherit from -- + # an OrcaSlicer preset-format gap, filed upstream as + # OrcaSlicer PR #13315, not something resolvable here. own_filament_id = detail.get("filament_id") or ( cloud_setting.get("filament_id") if isinstance(cloud_setting, dict) else None ) diff --git a/backend/tests/unit/test_orca_cloud_refresh.py b/backend/tests/unit/test_orca_cloud_refresh.py index 2d75d576d..a946ac665 100644 --- a/backend/tests/unit/test_orca_cloud_refresh.py +++ b/backend/tests/unit/test_orca_cloud_refresh.py @@ -8,6 +8,7 @@ still clear on that signal — a person is looking at the page and can pair agai pairing (#2717). """ +import asyncio from unittest.mock import AsyncMock, MagicMock, patch import pytest @@ -48,6 +49,7 @@ def _expired_service(refresh_side_effect=None): svc.refresh = AsyncMock(side_effect=refresh_side_effect) svc.access_token = "oc_ext_new" svc.token_expiry = None + svc.close = AsyncMock() return svc @@ -123,3 +125,106 @@ class TestSuccessfulRefresh: assert result.scalar_one().value == "oc_ext_new" result = await db_session.execute(select(Settings).where(Settings.key == _SETTINGS_KEYS["refresh_token"])) assert result.scalar_one().value == "oc_ext_rt_new" + + +class TestTheClientIsNotLeakedOnFailure: + """A built service owns an httpx client from construction. + + On success the caller closes it. On failure nobody is ever handed it, so + the builder has to close it itself -- otherwise every failed build leaks a + client into the connection pool. Harmless enough while the only callers + were routes, where a person retries a broken sign-in a handful of times; + it stopped being harmless once spool assignment started building one per + Orca-referenced spool, which fails on every assignment for as long as the + stored credentials cannot be refreshed. + """ + + @pytest.mark.asyncio + async def test_a_rejected_refresh_closes_it(self, db_session): + await _store_global_credentials(db_session) + svc = _expired_service(OrcaCloudAuthError("grant already used")) + + with ( + patch("backend.app.api.routes.orca_cloud.OrcaCloudService", return_value=svc), + pytest.raises(HTTPException), + ): + await _build_authenticated_service(db_session, None) + + svc.close.assert_awaited_once() + + @pytest.mark.asyncio + async def test_an_unreachable_orca_closes_it(self, db_session): + await _store_global_credentials(db_session) + svc = _expired_service(OrcaCloudError("connection reset")) + + with ( + patch("backend.app.api.routes.orca_cloud.OrcaCloudService", return_value=svc), + pytest.raises(HTTPException), + ): + await _build_authenticated_service(db_session, None, clear_on_auth_failure=False) + + svc.close.assert_awaited_once() + + @pytest.mark.asyncio + async def test_an_expired_token_with_nothing_to_refresh_closes_it(self, db_session): + """The earliest raise, before any network call -- and the one easiest + to miss, since it is a bare `raise` rather than an except block.""" + await _store_global_credentials(db_session) + svc = _expired_service() + svc.refresh_token = "" + + with ( + patch("backend.app.api.routes.orca_cloud.OrcaCloudService", return_value=svc), + pytest.raises(HTTPException) as exc, + ): + await _build_authenticated_service(db_session, None) + + assert exc.value.status_code == 401 + svc.refresh.assert_not_awaited() + svc.close.assert_awaited_once() + + @pytest.mark.asyncio + async def test_a_cancelled_build_closes_it_and_stays_cancelled(self, db_session): + """CancelledError is a BaseException, so an `except Exception` guard + would let the client leak on shutdown -- and swallowing it here would + break cancellation itself, which is the worse of the two bugs.""" + await _store_global_credentials(db_session) + svc = _expired_service(asyncio.CancelledError()) + + with ( + patch("backend.app.api.routes.orca_cloud.OrcaCloudService", return_value=svc), + pytest.raises(asyncio.CancelledError), + ): + await _build_authenticated_service(db_session, None) + + svc.close.assert_awaited_once() + + @pytest.mark.asyncio + async def test_a_failing_close_does_not_mask_the_real_error(self, db_session): + """Cleanup is best-effort. The caller needs the auth failure, not + whatever went wrong tidying up after it.""" + await _store_global_credentials(db_session) + svc = _expired_service(OrcaCloudAuthError("grant already used")) + svc.close = AsyncMock(side_effect=RuntimeError("pool already shut down")) + + with ( + patch("backend.app.api.routes.orca_cloud.OrcaCloudService", return_value=svc), + pytest.raises(HTTPException) as exc, + ): + await _build_authenticated_service(db_session, None) + + assert exc.value.status_code == 401 + + @pytest.mark.asyncio + async def test_a_successful_build_leaves_it_open_for_the_caller(self, db_session): + """The other half of the contract: closing here would hand back a dead + client and break every route that uses one.""" + await _store_global_credentials(db_session) + svc = _expired_service() + svc.refresh_token = "oc_ext_rt_new" + + with patch("backend.app.api.routes.orca_cloud.OrcaCloudService", return_value=svc): + returned = await _build_authenticated_service(db_session, None) + + assert returned is svc + svc.close.assert_not_awaited() diff --git a/frontend/src/components/ConfigureAmsSlotModal.tsx b/frontend/src/components/ConfigureAmsSlotModal.tsx index 379137dd8..1385015ad 100644 --- a/frontend/src/components/ConfigureAmsSlotModal.tsx +++ b/frontend/src/components/ConfigureAmsSlotModal.tsx @@ -521,10 +521,10 @@ export function ConfigureAmsSlotModal({ const detail = await api.getCloudSettingDetail(selectedPresetId); // The preset's own filament_id is what puts it in the slot as // itself — the printer stores that id and the slicer matches its - // presets against it. Bambu Cloud returns it on the envelope for - // some presets and inside the preset JSON under `setting` for - // others; looking only at the envelope is how a custom preset - // silently became its inherited base profile (#3003). + // presets against it. Bambu Cloud normally returns it on the + // envelope; the `setting` fallback covers a response that carries + // it in the preset JSON instead. A preset created in Orca has none + // in either place (#1053), and legitimately falls through. const nested = detail.setting?.filament_id; const ownFilamentId = detail.filament_id || (typeof nested === 'string' ? nested : ''); if (ownFilamentId) {