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) {