From 50c3e94d332ba4db9ab35c45b143b9d8bf40d8dc Mon Sep 17 00:00:00 2001 From: maziggy Date: Fri, 10 Jul 2026 08:24:20 +0200 Subject: [PATCH] fix(cloud): log expected preset misses at DEBUG, keep real faults at WARNING Failed to get cloud preset ... 400 {"message":"missing"} is the expected answer, not a fault: many official presets are only addressable with a printer-variant suffix (GFSL05 exists solely as GFSL05_07 @BBL A1), and personal P-prefixed presets belong to the account that sliced the file. Phase 3 already resolves both from local presets, so the lookup miss is routine -- and one WARNING per AMS tray per tooltip refresh teaches operators to ignore the log. BambuCloudError now carries the upstream status_code. The preset lookup logs HTTP 400 at DEBUG; expired tokens, 5xx and transport failures stay at WARNING. Not fixed here: resolving the variant suffix. It selects a printer profile and the response carries that profile's pressure_advance, so guessing a suffix would report another printer's K value. --- CHANGELOG.md | 3 +- backend/app/api/routes/cloud.py | 21 ++++- backend/app/services/bambu_cloud.py | 17 ++++- .../test_cloud_token_auth_migration.py | 76 +++++++++++++++++++ 4 files changed, 110 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ecd1baa4..402a73195 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,7 +37,8 @@ All notable changes to Bambuddy will be documented in this file. - **Sort File Manager folder tree by recent activity (#1770, requested by @Kingbuzz0)** — Until now the folder tree was always sorted alphabetically by name, both backend (`order_by(LibraryFolder.name)`) and frontend. The reporter — a user with a lot of nested cad / slicer directories — wanted "find folders that just got a new 3MF" without scrolling the whole alphabet. **What changed.** The folder sidebar header gains a small dropdown (**By name** / **By recent activity**) plus an asc / desc arrow button, sitting alongside the existing Collapse + Wrap toggles. Choice persists per-browser via `localStorage` (`library-folder-sort-field`, `library-folder-sort-direction`) so the preference survives reloads. **Activity semantics.** `latest_activity_at` per folder = `MAX(folder.updated_at, MAX(immediate-child file.updated_at))`. The DB had the data — `LibraryFile.updated_at` is `onupdate=func.now()` and `LibraryFolder.updated_at` the same — but `LibraryFolder.updated_at` alone only bumps on rename / move, not on file-add inside the folder, which is exactly the wrong signal for "did I just drop a new model in here." The aggregate fixes that. Recursion across subfolders is intentionally **NOT** computed — a deeply nested new 3MF bubbles its immediate parent, not every ancestor up to the root. This keeps the route a single `GROUP BY` rather than a recursive CTE, matching the existing file_counts subquery shape sibling at `library.py:746`. A future Tier 3 follow-up could add the recursive-CTE variant if anyone reports deeply-nested updates not bubbling far enough. **Backend.** New `latest_activity_at: datetime | None` field on `FolderResponse` and `FolderTreeItem` schemas. The `/folders` tree route picks up a sibling `func.max(LibraryFile.updated_at)` group-by alongside the existing file-count subquery; resolves the field per row. The `/folders/by-project/{id}` and `/folders/by-archive/{id}` routes collapse their per-row file-count subquery to fetch `count + max` in one trip (one extra column, zero extra round-trips). All 5 single-folder constructors (POST `/folders`, GET `/folders/{id}`, PUT `/folders/{id}`, POST `/folders/external`, the create flows) populate the field with `max(folder.updated_at, latest_file)` or fall back to `folder.updated_at` when there are no files, so the API surface is consistent across every route that returns a folder. **External folders.** `LibraryFile` rows are created for scanned external files too (`library.py:526`), so the MAX aggregate works on them — but the timestamp reflects when Bambuddy last *scanned / re-indexed* the file, not the filesystem mtime. For a NAS that gets new files added outside Bambuddy, the activity-sort lags until the next scan. Documented in the file-manager wiki page rather than papered over with `os.stat()` on every list call, which would stall the route on slow mounts. **Frontend.** A new recursive `sortedFolders` `useMemo` applies the comparator uniformly to top-level + every nested `children` level so sort order is consistent at every depth. Comparator falls back to name when activity timestamps tie or are both null, so an empty folder never elbows a recently-used one to a random place — empties go to the end of the activity bucket regardless of direction. Both the desktop sidebar render and the mobile selector dropdown consume `sortedFolders` so the order is identical across breakpoints. The single-folder `findFolder()` traversal and `selectedFolder` memo still operate on the unsorted `folders` because they index by ID — sort-order-independent. **Recursion safety.** The sort creates fresh object refs at every level on every memo invocation; the `FolderTreeItem` keys stay ID-based (`${folder.id}-${collapseFoldersByDefault ? 'c' : 'e'}`) so React reconciliation by ID preserves folder expansion state across sort flips. **i18n.** 3 new keys in `fileManager.*` (`folderSort`, `folderSortByName`, `folderSortByActivity`) translated in all 11 locales (de / en / es / fr / it / ja / ko / pt-BR / tr / zh-CN / zh-TW), no English fallback. Parity 5238 leaves per locale. **Tests.** 2 new backend integration cases in `test_library_api.py` (file-in-folder bubbles `latest_activity_at` to the file's timestamp, empty folder falls back to `folder.updated_at`). All 152 library + folder + trash + slice integration tests still pass; 51/51 FileManagerPage frontend tests still pass; 26/26 QueuePage tests still pass; `npm run build` clean; `ruff` clean; i18n parity green. ### Fixed -- **Enabling authentication silently disconnected Bambu Cloud (#2530, reporter @hburn7)** — Cloud credentials live in two different places depending on auth state: `get_stored_token()` reads the global `Settings` rows (`bambu_cloud_token` / `_email` / `_region`) when auth is off, and `User.cloud_token` when it's on. Completing `POST /auth/setup` flipped which store the `/cloud/*` routes consult, but nothing carried the token across — so an operator who linked their Bambu account *before* turning on auth found the freshly-created admin had `cloud_token = NULL`. `build_authenticated_cloud()` then returned `None` and every cloud route degraded: `get_filament_info` skipped its cloud phase entirely and answered `200` from the local-preset and built-in-name fallbacks, while `/cloud/devices` began returning `401`. Nothing surfaced the disconnect. The reporter observed the *symptom inverted* — cloud `400` warnings vanished after enabling auth — and reasonably read that as a fix; in fact the warnings stopped because Bambuddy had stopped calling the cloud at all. The tell is in their own timestamps: the pre-auth request spent ~960 ms on cloud round-trips, the post-auth one answered immediately. **Fix.** `setup_auth()` now migrates a globally-stored token onto the owning admin (and deletes the global rows, so a live credential isn't left at rest in a table nothing reads), and `disable_auth()` performs the mirror hand-off back to global storage. Both refuse to guess when ownership is ambiguous: setup migrates only when it creates the admin or exactly one admin already exists — with several admins it leaves the credential in place and logs a warning rather than handing one admin another's Bambu session; disable declines to overwrite a pre-existing global token. The `region` survives both hops rather than silently resetting to `global`. **Note for existing installs.** The migration runs at the auth on/off transition, so instances that already crossed it must re-link their Bambu account once from Settings → Bambu Cloud; the stranded `bambu_cloud_*` rows in `settings` can then be deleted. **Tests.** 7 integration cases pinning both directions, the two refuse-to-guess paths, the `auth_enabled=false` no-op, and region preservation. **Scope.** No DB migration, no schema change, no new permission, no i18n change. Separately tracked: the cloud `400 missing` warnings themselves are unrelated to auth and are logged at WARNING for an expected, already-handled fallback. +- **Enabling authentication silently disconnected Bambu Cloud (#2530, reporter @hburn7)** — Cloud credentials live in two different places depending on auth state: `get_stored_token()` reads the global `Settings` rows (`bambu_cloud_token` / `_email` / `_region`) when auth is off, and `User.cloud_token` when it's on. Completing `POST /auth/setup` flipped which store the `/cloud/*` routes consult, but nothing carried the token across — so an operator who linked their Bambu account *before* turning on auth found the freshly-created admin had `cloud_token = NULL`. `build_authenticated_cloud()` then returned `None` and every cloud route degraded: `get_filament_info` skipped its cloud phase entirely and answered `200` from the local-preset and built-in-name fallbacks, while `/cloud/devices` began returning `401`. Nothing surfaced the disconnect. The reporter observed the *symptom inverted* — cloud `400` warnings vanished after enabling auth — and reasonably read that as a fix; in fact the warnings stopped because Bambuddy had stopped calling the cloud at all. The tell is in their own timestamps: the pre-auth request spent ~960 ms on cloud round-trips, the post-auth one answered immediately. **Fix.** `setup_auth()` now migrates a globally-stored token onto the owning admin (and deletes the global rows, so a live credential isn't left at rest in a table nothing reads), and `disable_auth()` performs the mirror hand-off back to global storage. Both refuse to guess when ownership is ambiguous: setup migrates only when it creates the admin or exactly one admin already exists — with several admins it leaves the credential in place and logs a warning rather than handing one admin another's Bambu session; disable declines to overwrite a pre-existing global token. The `region` survives both hops rather than silently resetting to `global`. **Note for existing installs.** The migration runs at the auth on/off transition, so instances that already crossed it must re-link their Bambu account once from Settings → Bambu Cloud; the stranded `bambu_cloud_*` rows in `settings` can then be deleted. **Tests.** 7 integration cases pinning both directions, the two refuse-to-guess paths, the `auth_enabled=false` no-op, and region preservation. **Scope.** No DB migration, no schema change, no new permission, no i18n change. +- **Routine cloud preset misses no longer log at WARNING (#2530)** — The `Failed to get cloud preset … 400 {"message":"missing"}` lines that led to #2530 being filed are an *expected* answer, not a fault, and `get_filament_info`'s Phase 3 already resolves the name from local presets (a bare `GFL05` lands on "Overture Matte PLA" without the cloud). Two routine causes, both confirmed against the live Bambu catalog: many official presets are only addressable with a printer-variant suffix — `GFSA00` and `GFSL99` resolve bare, but `GFSL05` and `GFSG00` exist *only* as `GFSL05_07` (`@BBL A1`), `GFSG00_06` and so on, while the AMS reports the bare ID; and personal presets (`P…`, e.g. `Pb5b7d17`) belong to whichever Bambu account sliced the file, so no other account will ever resolve them. Emitting a WARNING per tray on every AMS tooltip refresh trains operators to ignore the log. `BambuCloudError` now carries the upstream `status_code`, and the preset lookup logs an HTTP 400 at DEBUG while leaving every other failure — expired token, 5xx, connection error — at WARNING, so a genuine fault is still loud. Deliberately *not* fixed here: resolving the variant suffix. The suffix selects a printer profile, and the endpoint returns `pressure_advance` (the K value), which is per-printer — picking a suffix arbitrarily would populate AMS tooltips with another printer's K value, which is worse than the current blank. Doing that correctly requires threading the tray's printer model into `get_filament_info`, which changes the endpoint contract. **Tests.** 4 parametrised cases drive the real route with a stubbed cloud and assert the 400 lands at DEBUG while 401 / 502 / transport failures stay at WARNING; verified by mutation (forcing the classification off fails the 400 case). - **Printer FTPS and MQTT connections inherited their TLS floor from the OpenSSL build instead of declaring one** — `ImplicitFTP_TLS` (`bambu_ftp.py`) and the MQTT client (`bambu_mqtt.py`) both built their context with `ssl.create_default_context()`, which leaves `minimum_version` at `MINIMUM_SUPPORTED`. What that resolves to is a property of the interpreter's OpenSSL build, not of Bambuddy: measured on identical `OpenSSL 3.5.6`, the `python:3.13-slim-trixie` Docker base reports `TLSVersion.TLSv1_2` while a bare-metal venv reports `MINIMUM_SUPPORTED` — so Docker users have always been floored at TLS 1.2, while bare-metal and appliance installs could in principle negotiate TLS 1.0 or 1.1 with a printer that offered them. **Fix.** Both contexts now set `minimum_version = ssl.TLSVersion.TLSv1_2` explicitly. On the two FTP profiles that also cap `maximum_version` (P2S, X2D — see #1401) this yields an exact TLS 1.2 pin rather than a ceiling over an inherited floor. **Verified against hardware, not just tests.** Probing an X1C and an H2D on both `:990` and `:8883`, each printer completes only on TLS 1.2 and rejects 1.0, 1.1 *and* 1.3 with a `handshake_failure` alert; a live FTPS login through the changed code path succeeds on both with `TLSv1.2` negotiated. Since the shipped Docker image already enforced this floor across the whole install base, no printer or firmware reachable today can be affected by making it explicit. **Also corrected** a stale comment in `ftp_profiles.py` claiming X1C / H2D installs "stay on the negotiated TLS 1.3" — both models refuse 1.3 outright, so `cap_tls_v1_2` is a no-op there; the P2S evidently does offer 1.3, which is why it alone surfaced the vsFTPd session-reuse bug. **Scope.** Two lines plus a comment. No behaviour change on Docker, no DB migration, no new permission, no i18n change; certificate verification is unchanged (printers use self-signed certs, so `check_hostname`/`CERT_NONE` remain by necessity). - **Backend failed to start on fastapi < 0.116: `AssertionError: Status code 204 must not have a response body`** — `uvicorn backend.app.main:app` aborted at import time while registering `DELETE /api/v1/library/tags/{tag_id}`. The route is declared `status_code=204` with a `-> None` return annotation, and `library_tags.py` uses `from __future__ import annotations` — so the annotation reaches FastAPI as the *string* `"None"`, which `get_typed_annotation()` resolves via `evaluate_forwardref()` to `NoneType`. `NoneType` is a class and therefore truthy, so `APIRoute.__init__` took the `if self.response_model:` branch and asserted that a 204 may carry no response body. fastapi **0.116** added an `if annotation is type(None): return None` guard that makes this benign, which is why CI and the Docker image (both resolve the top of the `>=0.109.0,<0.136.0` range) never saw it — only installs pinned to an older release inside that supported range, such as a venv created before the tag catalog landed in #1268, hit the crash. **Fix.** The route declares `response_model=None` explicitly, which short-circuits the annotation inference on every fastapi version in the supported range. The sibling 204 route (`DELETE /slicer/pipelines/{pipeline_id}`) is unaffected — its module has no `from __future__ import annotations` and no return annotation. **Scope.** Backend-only, one decorator. No behaviour change on fastapi >= 0.116, no DB migration, no new permission, no i18n change. Existing installs can equivalently unblock themselves with `pip install -U -r requirements.txt`. - **Dependency floors permitted resolutions the code can't run on: `sqlalchemy>=2.0.38`, exact ruff pin** — Two more instances of the same class of defect as the 204 crash above: `requirements.txt` declared floors low enough that a legitimate `pip install -r requirements.txt` could produce an environment Bambuddy fails to start or lint in. CI never caught either, because a fresh runner always resolves to the *top* of every range — only a longer-lived venv resolving lower hits them. **sqlalchemy.** `core/database._create_engine()` passes `pool_size` / `max_overflow` on the SQLite branch. SQLAlchemy **2.0.38** changed the aiosqlite dialect's default pool for file databases from `NullPool` (which rejects both kwargs) to `AsyncAdaptedQueuePool` (which accepts them); on 2.0.0-2.0.37 the module-level `engine = _create_engine()` raises `TypeError: Invalid argument(s) 'pool_size','max_overflow' sent to create_engine()` at import, taking down every SQLite install and the whole test suite (`conftest.py` imports the module). Postgres installs were never affected — `is_sqlite()` is False and the branch is dead. Floor raised to `sqlalchemy>=2.0.38`. **ruff.** The lint job ran a bare `pip install ruff` (always the newest release) while `requirements-dev.txt` said `ruff>=0.8.0`, so CI's linter and a contributor's were routinely *different programs enforcing different rule sets*. A venv holding ruff 0.8.4 reported 32 errors against a tree current ruff calls clean — 30 of them `UP038`, a rule ruff has since **removed** (PEP 604 syntax in `isinstance()` is slower than the tuple form it wanted you to replace). ruff is now pinned exactly (`ruff==0.15.20`) and the CI lint job installs that pin from `requirements-dev.txt`, so local and CI enforce the same rules and `format --check` can't disagree across machines. **Scope.** Packaging + CI only; no application code, no DB migration, no permission, no i18n change. Existing environments should re-run `pip install -U -r requirements.txt -r requirements-dev.txt`. diff --git a/backend/app/api/routes/cloud.py b/backend/app/api/routes/cloud.py index ae06abb3d..dcdf12690 100644 --- a/backend/app/api/routes/cloud.py +++ b/backend/app/api/routes/cloud.py @@ -861,9 +861,24 @@ async def get_filament_info( if not name: still_unresolved.append(setting_id) except Exception as e: - logger.warning( - f"Failed to get cloud preset {setting_id} " - f"(API ID: {_filament_id_to_setting_id(setting_id)}): {e}" + # A 400 here is the *expected* answer, not a fault, and Phase 3 + # exists to handle it (#2530). Two routine causes: + # * Many official presets are only addressable with a printer + # variant suffix — "GFSA00" resolves, "GFSL05" does not, only + # "GFSL05_07" (@BBL A1) does. The bare ID is all the AMS + # reports, so the lookup legitimately misses. + # * Personal presets ("P…") belong to the Bambu account that + # sliced the file; another account will never resolve them. + # Logging those at WARNING on every AMS tooltip refresh trains + # users to ignore the log. Anything else — expired token, 5xx, + # a connection failure — stays at WARNING because it is a fault. + expected_miss = isinstance(e, BambuCloudError) and e.status_code == 400 + logger.log( + logging.DEBUG if expected_miss else logging.WARNING, + "Failed to get cloud preset %s (API ID: %s): %s", + setting_id, + _filament_id_to_setting_id(setting_id), + e, ) still_unresolved.append(setting_id) diff --git a/backend/app/services/bambu_cloud.py b/backend/app/services/bambu_cloud.py index 7ea43701b..f2778a2bb 100644 --- a/backend/app/services/bambu_cloud.py +++ b/backend/app/services/bambu_cloud.py @@ -79,9 +79,17 @@ _SLICER_API_VERSION = "1.0.0.0" class BambuCloudError(Exception): - """Base exception for Bambu Cloud errors.""" + """Base exception for Bambu Cloud errors. - pass + ``status_code`` carries the upstream HTTP status when the failure came from + a response rather than from the transport, so callers can tell an expected + "this preset isn't in the catalog" 400 apart from an expired token or a + cloud outage. It stays ``None`` for connection-level failures. + """ + + def __init__(self, message: str, *, status_code: int | None = None): + super().__init__(message) + self.status_code = status_code class BambuCloudAuthError(BambuCloudError): @@ -408,7 +416,10 @@ class BambuCloudService: # Include body so a future contract change is self-diagnostic from logs. body = (response.text or "")[:200] - raise BambuCloudError(f"Failed to get setting detail: {response.status_code} {body}") + raise BambuCloudError( + f"Failed to get setting detail: {response.status_code} {body}", + status_code=response.status_code, + ) except httpx.RequestError as e: raise BambuCloudError(f"Request failed: {e}") diff --git a/backend/tests/integration/test_cloud_token_auth_migration.py b/backend/tests/integration/test_cloud_token_auth_migration.py index c4cc09462..c1e97d769 100644 --- a/backend/tests/integration/test_cloud_token_auth_migration.py +++ b/backend/tests/integration/test_cloud_token_auth_migration.py @@ -12,11 +12,14 @@ These tests pin the hand-off in both directions, and — just as importantly — pin the two cases where Bambuddy must refuse to guess who owns a credential. """ +import logging + import pytest from httpx import AsyncClient from sqlalchemy import select from sqlalchemy.ext.asyncio import AsyncSession +from backend.app.api.routes import cloud as cloud_routes from backend.app.api.routes.cloud import ( CLOUD_EMAIL_KEY, CLOUD_REGION_KEY, @@ -26,6 +29,7 @@ from backend.app.api.routes.cloud import ( from backend.app.core.auth import get_password_hash from backend.app.models.settings import Settings from backend.app.models.user import User +from backend.app.services.bambu_cloud import BambuCloudError async def _seed_global_token(db: AsyncSession, token: str = "tok-global", region: str = "china") -> None: @@ -199,3 +203,75 @@ async def test_disable_auth_with_no_cloud_token_is_a_noop(async_client: AsyncCli resp = await async_client.post("/api/v1/auth/disable", headers={"Authorization": f"Bearer {bearer}"}) assert resp.status_code == 200, resp.text assert await _global_rows(db_session) == {} + + +# --------------------------------------------------------------------------- +# Log-level classification for cloud preset misses (#2530) +# --------------------------------------------------------------------------- + + +def test_bambu_cloud_error_carries_status_code(): + """The 400-vs-fault distinction depends on this attribute existing.""" + from backend.app.services.bambu_cloud import BambuCloudError + + assert BambuCloudError("boom").status_code is None + assert BambuCloudError("boom", status_code=400).status_code == 400 + # Every pre-existing raise site passes a bare message; must not break. + assert str(BambuCloudError("Request failed: timeout")) == "Request failed: timeout" + + +class _StubCloud: + """Minimal stand-in for BambuCloudService that fails every preset lookup.""" + + def __init__(self, error: Exception): + self._error = error + self.is_authenticated = True + + async def get_setting_detail(self, setting_id: str) -> dict: + raise self._error + + async def close(self) -> None: + return None + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + ("error", "expected_level"), + [ + (BambuCloudError("missing", status_code=400), logging.DEBUG), + (BambuCloudError("unauthorized", status_code=401), logging.WARNING), + (BambuCloudError("bad gateway", status_code=502), logging.WARNING), + (BambuCloudError("Request failed: timeout"), logging.WARNING), + ], + ids=["expected-400-miss", "expired-token", "cloud-outage", "transport-failure"], +) +async def test_preset_miss_logs_at_debug_but_faults_stay_at_warning( + monkeypatch, caplog, db_session: AsyncSession, error: Exception, expected_level: int +): + """A preset the catalog doesn't carry is routine; an expired token is a fault. + + Drives the real ``get_filament_info`` route so the classification is exercised + where it lives, not re-derived in the test. + """ + monkeypatch.setattr(cloud_routes, "build_authenticated_cloud", _stub_builder(error)) + # Phase 1 would otherwise short-circuit the cloud call on a warm cache. + monkeypatch.setattr(cloud_routes, "_filament_cache", {}) + + with caplog.at_level(logging.DEBUG, logger="backend.app.api.routes.cloud"): + result = await cloud_routes.get_filament_info(setting_ids=["GFL05"], db=db_session, current_user=None) + + records = [r for r in caplog.records if "Failed to get cloud preset" in r.getMessage()] + assert len(records) == 1, "the miss must be logged exactly once" + assert records[0].levelno == expected_level + assert "GFL05" in records[0].getMessage() + assert "GFSL05" in records[0].getMessage(), "the translated API ID stays in the message" + + # Whatever the level, the endpoint still answers and falls through to Phase 3. + assert isinstance(result, dict) + + +def _stub_builder(error: Exception): + async def _build(db, user): + return _StubCloud(error) + + return _build