From 89c4ac5583c39e8310608fe8eff41e816ddd7e54 Mon Sep 17 00:00:00 2001 From: maziggy Date: Mon, 28 Sep 2026 10:48:51 +0200 Subject: [PATCH] Post work PR #2956 ----- Give each test worker its own scratch folder in the #3025 and #3029 tests (issue #3025) Both modules wrote into one shared folder under settings.base_dir and deleted it after every test. Under xdist, one worker's cleanup removed files another worker was still serving, so tests failed at random with a 404. The folder name now includes the process ID. --- CHANGELOG.md | 2 +- backend/tests/integration/test_media_token_3025.py | 7 ++++++- backend/tests/integration/test_slicer_token_reuse_3029.py | 7 +++++-- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b1687bacb..b2f3819ac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -140,7 +140,7 @@ All notable changes to Bambuddy will be documented in this file. - **A clear spool synced to Spoolman as pure black (#2912, reported and contributed by @ojimpo in #2924)** — The AMS reports a translucent roll as `00000000`, and every write to Spoolman truncated that to six characters before storing it, so a PETG Translucent spool arrived as opaque black and the external catalogue then named it "Black". Spoolman's own schema accepts eight characters, so the value Bambuddy was discarding was one the backend would have taken verbatim. #1545 fixed exactly this for the built-in inventory and left the Spoolman path behind, which is why internal mode has been storing the alpha correctly for months. The read side was the matching half: it rejected anything that was not exactly six characters, so fixing the writes alone would have turned clear spools grey instead of black. Eight characters are stored only when the alpha byte says the filament is genuinely translucent — passing everything through would rewrite the colour of every opaque spool on its next touch, churning records in people's Spoolman for no benefit. Colour comparisons now key on the shape a value would be stored as, so two colours match exactly when storing them would produce the same value. That is what keeps the widening safe in both directions: an opaque tray still finds the six-character filaments every existing instance is full of, so no upgrade mints a duplicate for every spool on the next sync, while a clear roll gets its own record instead of being conflated with the black one of the same RGB. The edit route compares the same way, so a no-op edit no longer PATCHes the filament on every save and an alpha-only edit still reaches Spoolman. One consequence is worth stating: a filament already stored wrongly-opaque by this bug gets a second, correct record the next time that roll is auto-added, rather than the old one silently capturing every clear spool that follows. - **Translucent spools showed as an empty circle or as solid black in four more places** — The swatch helper had two answers, the transparency checkerboard for a fully clear colour and a flat fill for everything else, so a half-translucent spool rendered identically to an opaque one. The AMS tray swatches never reached that helper at all: Assign Spool painted the reported colour directly, so a clear tray was an invisible circle, and Configure AMS Slot cut the alpha off first, so a clear tray was solid black — the same symptom as the sync bug above, in the UI, and present regardless of which inventory mode is in use. All of them now draw through one helper, which lays a partly translucent colour over the checkerboard so the swatch shows both the tint and that it is see-through. - **Every notification provider vanished from the list after the inventory toggles were wired up** — Adding `on_stock_reorder_alert` and `on_stock_break_alert` to the provider schema made them required on the way out as well as the way in, because the response model inherits the write model. Every `on_*` column on `notification_providers` is nullable with no server default, and on an install where the table had already been created from the ORM metadata before migrations ran, the `ALTER ... DEFAULT false` that introduced those two columns was swallowed as a duplicate and never backfilled the rows that were already there. Those NULLs sat harmless for as long as nothing read them; the moment the flags were declared on the response, the row failed validation, and since a list is validated as a whole, one such row took every provider down with it. The API returned a 500 and the UI rendered what it was given — an empty list — so correctly configured providers looked deleted while sitting untouched in the database. They are backfilled to off on the next start, matching what the sender already did with them: it selects providers with `IS TRUE`, so a NULL flag never sent anything. A NULL flag now also reads as off rather than failing the response, so the next flag added to that schema cannot repeat this. -- **Two inventory notification toggles could never be turned on, so stock alerts have never been able to fire** — `on_stock_reorder_alert` and `on_stock_break_alert` exist as columns on a notification provider, have their own templates, and `notification_service` looks providers up under exactly those names before sending. The whole UI is there too: a toggle in Add/Edit Notification, a badge on the provider card, the field in the API client's types, and tests for all of it. The one thing missing was the schema. `NotificationProviderCreate`/`Update` never declared either field, and Pydantic drops what it does not declare, so every request that carried them came back `200 OK` with the row unchanged — and `_provider_to_dict`, which is a hand-maintained field-by-field map, never returned them either, so the toggle read back off no matter what the database held. Nothing errored anywhere along that path. Both directions are wired now, and the round-trip tests that already covered the Home Assistant toggles cover these too, because the failure is structural rather than particular to one field: any column missing from those two maps is invisible to a test that builds providers through the ORM, and only a create-then-re-read through the route catches it. This makes the setting stick and report itself honestly; the detection side that would *call* those two senders does not exist yet, so turning them on does not yet produce notifications. +- **Two inventory notification toggles could never be turned on, so stock alerts have never been able to fire (#2945, reported by @ojimpo; regression tests contributed by @ojimpo in #2956)** — `on_stock_reorder_alert` and `on_stock_break_alert` exist as columns on a notification provider, have their own templates, and `notification_service` looks providers up under exactly those names before sending. The whole UI is there too: a toggle in Add/Edit Notification, a badge on the provider card, the field in the API client's types, and tests for all of it. The one thing missing was the schema. `NotificationProviderCreate`/`Update` never declared either field, and Pydantic drops what it does not declare, so every request that carried them came back `200 OK` with the row unchanged — and `_provider_to_dict`, which is a hand-maintained field-by-field map, never returned them either, so the toggle read back off no matter what the database held. Nothing errored anywhere along that path. Both directions are wired now, and the round-trip tests that already covered the Home Assistant toggles cover these too, because the failure is structural rather than particular to one field: any column missing from those two maps is invisible to a test that builds providers through the ORM, and only a create-then-re-read through the route catches it. This makes the setting stick and report itself honestly; the detection side that would *call* those two senders does not exist yet, so turning them on does not yet produce notifications. The tests no longer depend on anyone remembering the next field, either. They now read the list of provider fields from the database model itself, so a toggle or setting added later is checked from the day its column exists, with no new test to write. It must be accepted when a provider is created and when it is edited, and every toggle must survive a save and read back from both the single-provider and the list endpoint. Each toggle is set to the opposite of its default for that check, because nine of them default to on and a test that saves "on" and reads back "on" would pass even if the value were silently dropped. - **One Home Assistant sensor reporting a long text state could stop every printer sensor from updating** — `last_state` is a 64-character column, and the poller wrote whatever Home Assistant returned straight into it. A numeric entity that starts answering with free text - an enum, an error string from a template sensor - overflows that. SQLite stores it regardless, which is why this stayed quiet, but PostgreSQL rejects the row, and a poll pass commits every sensor at once: one such entity took the whole batch down on every tick, so no printer sensor's reading, timestamp or alert state advanced again, and the print interlock kept deciding against a frozen picture. What is persisted is now cut to the column, while the cached reading keeps the full state for display. The comparison that decides whether the state changed is made against the cut form too - comparing the stored value against the raw one would read as a difference on every single poll and churn `last_changed` forever. The storage-location poller was fixed the same way in the same release; both now go through one helper, each passing its own table's width. - **Configure Slot could bind the default K value for a profile the picker was visibly showing** — The AMS slot dialog sends `cali_idx` from `selectedKProfile`, which the mutation read through its own closure. React Query hands a mutation its options from an *effect*, so a click landing between a commit and that effect flushing runs the previous render's function - one that captured the selection as it was before the K-profile query resolved. The result is `cali_idx: -1`: the printer binds the default 0.020 rather than the calibrated K, while the dialog shows the right profile selected the whole time. It surfaced as an intermittent failure of the per-nozzle K-profile test, roughly one full-suite run in six, and reproducing it with staggered query resolution showed the divergence directly - the select element held the correct profile immediately before and after the click, and the payload still carried -1. That test's slot is the most exposed case in the file, a right-hotend slot carrying the left hotend's index, where the "keep showing the active profile" safety net cannot repair an empty recompute. The mutation now reads the selection from a ref written during render, so it resolves at execute time rather than at capture time; the same applies to the K value and the profile's ids, which travel in the same payload and had the same exposure. Measured over 27 runs of a staggered-resolution grid: 2 failures in 15 before, 0 in 12 after. The modal's printer-model query was also missing from the test file's mock, so it ran with no query function and rejected on every test in it - mocked now, though on its own that changed nothing, which is how the ref was confirmed as the fix rather than assumed. - **A print started from the printer's own screen swept every FTP path, archived blank, and then blamed a slicer setting (#1820, reported, captured and re-measured across four daily builds by @ojimpo)** — `current_project_url` was assigned in exactly one place, `_handle_request_message`, and that runs only for the request topic. A print started from the touchscreen publishes nothing there, so the field stayed empty for the one case the storage verdict exists for: the file is already in the printer's own model library under `/userdata/model/history/`, and port 990 does not serve it. The verdict then fell through to the `sdcard` flag — which @ojimpo's H2S reports as true, its "card" being the internal eMMC — so every such print ran the full sweep before giving up: 16 filename-and-directory attempts over 22 FTPS connections, 18 of them refused, 6.4 seconds, then a fallback archive with a name and nothing else. The printer does announce where the file lives. It arrives as an unsolicited `project_file` **response** on the report topic about two seconds before `gcode_state` reaches PREPARE, and Bambuddy now reads the `url` off it, gated on `result: SUCCESS` and a non-empty value so a refused dispatch cannot name a file that was never written. Reading it there rather than only at the request topic also covers an install nobody had in view: some brokers refuse the request-topic subscription, and on those no print of any kind had ever populated the field. Our own dispatch is echoed on both topics, so the new branch captures state and nothing else — the "external dispatch" diagnostic stays with the request-topic handler, which sees the echo first, and reusing it here would have logged every Bambuddy-started print as somebody else's. What the print names is now what gets tried: the five directories a copy could be in, rather than the ~110 connections that cannot succeed. That copy is worth trying, because an H2S keeps recently used jobs under `/cache` and archives them in full while they last; roughly eight files later the same job archives with a name only, which is exactly what @ojimpo measured on two prints the same day. Nothing about slicer-sent prints changed. **The banner that appears afterwards no longer describes a step that never happened.** With no reason recorded, a blank archive fell back to the original wording — "Store sent files on external storage" is off in your slicer, go and turn it on. On the reporter's printer that setting is on, and the internal-storage wording added in #2780 already explains why it would not help on an H2 anyway, so the archive that most needed that explanation was the only one that could not be given it. Prints started from the screen, from Handy, or by picking a file the printer already had now carry a reason of their own and their own wording: no slicer was involved, nothing was sent, and no setting changes it. What can be done instead is offered in its place — start the print from Bambuddy, or read the sliced weight off the printer's own file browser and enter it under **Filament used (g)** in Edit Archive, which the cost and the Projects totals then follow. Settings > Printers > Connection Diagnostic reads the same reason rather than a fixed one, so the two surfaces cannot end up giving the same printer different advice. diff --git a/backend/tests/integration/test_media_token_3025.py b/backend/tests/integration/test_media_token_3025.py index b24625927..72df7b7e5 100644 --- a/backend/tests/integration/test_media_token_3025.py +++ b/backend/tests/integration/test_media_token_3025.py @@ -18,6 +18,7 @@ its header-authenticated siblings. from __future__ import annotations +import os import shutil from pathlib import Path @@ -108,7 +109,11 @@ async def _mint_camera_token(async_client: AsyncClient, jwt: str) -> str: # The routes resolve thumbnails relative to ``settings.base_dir``, so the # fixtures have to write there rather than into tmp_path. Keep them in one # subdirectory and delete it after every test so a run leaves the tree clean. -_THUMB_DIR = "test_thumbs_3025" +# +# ``base_dir`` is shared by every xdist worker, so the subdirectory is per +# process: with one shared name, a worker's teardown deleted the thumbnails +# another worker's test was about to serve, and that test got a 404. +_THUMB_DIR = f"test_thumbs_3025_{os.getpid()}" @pytest.fixture(autouse=True) diff --git a/backend/tests/integration/test_slicer_token_reuse_3029.py b/backend/tests/integration/test_slicer_token_reuse_3029.py index 84f5b700a..5d8217de8 100644 --- a/backend/tests/integration/test_slicer_token_reuse_3029.py +++ b/backend/tests/integration/test_slicer_token_reuse_3029.py @@ -27,6 +27,7 @@ route's own token check ever ran. from __future__ import annotations +import os import shutil from datetime import datetime, timedelta, timezone from pathlib import Path @@ -38,8 +39,10 @@ pytestmark = [pytest.mark.asyncio, pytest.mark.integration] # Same reasoning as #3025's fixtures: the routes resolve paths relative to # ``settings.base_dir``, which under test is the project root, so everything -# goes in one subdirectory that is removed after each test. -_FILE_DIR = "test_files_3029" +# goes in one subdirectory that is removed after each test. Per process for the +# same reason too: every xdist worker shares ``base_dir``, and with one shared +# name a worker's teardown deleted files another worker's test was serving. +_FILE_DIR = f"test_files_3029_{os.getpid()}" @pytest.fixture(autouse=True)