Bläddra i källkod

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.
maziggy 8 timmar sedan
förälder
incheckning
f3b1c59169

Filskillnaden har hållts tillbaka eftersom den är för stor
+ 1 - 1
CHANGELOG.md


+ 40 - 22
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.")
         raise HTTPException(status_code=401, detail="Orca Cloud is not connected — sign in first.")
 
 
     svc = OrcaCloudService()
     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:
         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
     return svc
 
 
 
 

+ 15 - 6
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
                     # gets it into an AMS slot as itself: the printer stores
                     # that id, the slicer matches its presets against it, and
                     # that id, the slicer matches its presets against it, and
                     # the 8-character field fits it exactly ("P" + 7 hex).
                     # 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 (
                     own_filament_id = detail.get("filament_id") or (
                         cloud_setting.get("filament_id") if isinstance(cloud_setting, dict) else None
                         cloud_setting.get("filament_id") if isinstance(cloud_setting, dict) else None
                     )
                     )

+ 105 - 0
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).
 pairing (#2717).
 """
 """
 
 
+import asyncio
 from unittest.mock import AsyncMock, MagicMock, patch
 from unittest.mock import AsyncMock, MagicMock, patch
 
 
 import pytest
 import pytest
@@ -48,6 +49,7 @@ def _expired_service(refresh_side_effect=None):
     svc.refresh = AsyncMock(side_effect=refresh_side_effect)
     svc.refresh = AsyncMock(side_effect=refresh_side_effect)
     svc.access_token = "oc_ext_new"
     svc.access_token = "oc_ext_new"
     svc.token_expiry = None
     svc.token_expiry = None
+    svc.close = AsyncMock()
     return svc
     return svc
 
 
 
 
@@ -123,3 +125,106 @@ class TestSuccessfulRefresh:
         assert result.scalar_one().value == "oc_ext_new"
         assert result.scalar_one().value == "oc_ext_new"
         result = await db_session.execute(select(Settings).where(Settings.key == _SETTINGS_KEYS["refresh_token"]))
         result = await db_session.execute(select(Settings).where(Settings.key == _SETTINGS_KEYS["refresh_token"]))
         assert result.scalar_one().value == "oc_ext_rt_new"
         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()

+ 4 - 4
frontend/src/components/ConfigureAmsSlotModal.tsx

@@ -521,10 +521,10 @@ export function ConfigureAmsSlotModal({
             const detail = await api.getCloudSettingDetail(selectedPresetId);
             const detail = await api.getCloudSettingDetail(selectedPresetId);
             // The preset's own filament_id is what puts it in the slot as
             // 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
             // 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 nested = detail.setting?.filament_id;
             const ownFilamentId = detail.filament_id || (typeof nested === 'string' ? nested : '');
             const ownFilamentId = detail.filament_id || (typeof nested === 'string' ? nested : '');
             if (ownFilamentId) {
             if (ownFilamentId) {

Vissa filer visades inte eftersom för många filer har ändrats