fix(archives): handle fallback archives in source-3MF upload (#1531)

Archives created from prints Bambuddy didn't archive (cloud / Handy /
  SD-card prints) carry file_path="". The two source-upload routes
  computed the destination as (base_dir / archive.file_path).parent /
  "source", which collapsed to base_dir.parent / "source" for fallback
  rows — sending the file to /app/source/ (outside the data volume,
  orphaned on container restart) and raising 500 on the final
  relative_to.

  Centralise the destination math in _resolve_source_3mf_path. Normal
  archives keep the <archive>/source/<filename> layout. Fallback
  archives land at <base_dir>/archive/no_source/<id>/<filename>, which
  stays inside the data volume and is addressable by every existing
  read site. The helper also asserts the resolved directory is under
  base_dir.resolve() so a corrupted row fails with a clear message
  instead of writing outside the volume.

  Both upload routes (upload_source_3mf and upload_source_3mf_by_name)
  now route through the helper. Two regression tests in
  TestUploadSourceThreeMF pin both branches.
This commit is contained in:
maziggy
2026-05-26 11:04:46 +02:00
parent 4387a09162
commit e9beb1e8fc
3 changed files with 194 additions and 18 deletions
+1
View File
@@ -29,6 +29,7 @@ All notable changes to Bambuddy will be documented in this file.
- **Trivy DS-0026 (`Dockerfile.test` missing HEALTHCHECK): silenced via `HEALTHCHECK NONE`** — The test image runs `pytest` and exits; there is no long-running service to probe, so any HEALTHCHECK we added would be cargo-cult noise. `HEALTHCHECK NONE` is the documented Docker directive to explicitly opt out of any inherited healthcheck and is the way Trivy expects projects to signal "this image is not a service." Closes code-scanning alert #813.
### Fixed
- **Source-3MF upload on "fallback" archives no longer crashes with HTTP 500 (and stops orphaning files outside the data volume) (#1531, reported by @d3nn3s08)** — When MQTT reports a print start but Bambuddy never saw the source 3MF (cloud-initiated prints, Bambu Handy, prints already on the printer's SD card when Bambuddy connected), `main.py:2596` creates a "fallback" `PrintArchive` row with `file_path=""`. The two `Archives → Source 3MF Upload` routes computed the destination directory as `(settings.base_dir / archive.file_path).parent / "source"` — which on a fallback row collapsed to `Path('/app/data') / '' = Path('/app/data')`, whose `.parent` is `Path('/app')`, sending the upload to `/app/source/<filename>.3mf`. The file was physically written there (a path outside the user's mounted data volume — orphaned on container restart) and only the *final* `source_path.relative_to(settings.base_dir)` raised, so every retry left another orphan. Affected reporter is on a QNAP Docker host with the standard `/app/data` mount; both maintainer and triage initially diagnosed it as a Docker volume misconfiguration, but the traceback shows the bug is purely on Bambuddy's side — the user's setup was correct. **Fix**: new private helper `_resolve_source_3mf_path(archive, source_filename)` in `backend/app/api/routes/archives.py` centralises the destination computation. Normal archives still nest the source under `<archive_file_dir>/source/<filename>`. Fallback archives (empty `file_path`) now land under `<base_dir>/archive/no_source/<archive_id>/<filename>` instead — a deterministic, addressable location that stays inside the data volume, and the existing read sites (`download_source_3mf`, `download_source_3mf_by_filename`, the slicer-token routes, `delete_source_3mf`) all continue to work because they read back via `settings.base_dir / archive.source_3mf_path`. The helper also defensively asserts the resolved directory is inside `base_dir.resolve()` regardless of where it came from, so a row corrupted by an old import or a manual SQL edit fails with a clear 500 message ("Archive N resolves to a path outside the data directory; cannot attach source.") instead of silently writing outside the volume. Both upload sites (`upload_source_3mf` and `upload_source_3mf_by_name`, the slicer-post-processing endpoint) now route through the helper, so neither can independently drift back into the bug. **Tests**: 2 new in `TestUploadSourceThreeMF` in `backend/tests/integration/test_archives_api.py` — (a) `test_fallback_archive_source_upload_lands_under_base_dir` creates an archive with `file_path=""`, uploads a minimal valid 3MF, asserts 200 status, that the returned `source_3mf_path` is relative (not `/app/source/...`), that the file physically exists under the patched `base_dir`, and that the path is the deterministic fallback location keyed off `archive.id`; (b) `test_normal_archive_source_upload_unchanged` is the same flow against an archive with a populated `file_path`, asserting the existing `archives/test/source/<filename>.3mf` layout is preserved (regression guard against the helper accidentally changing the normal path). 57/57 in `test_archives_api.py` green under `pytest -n 30`. Backend ruff clean. **Note**: existing orphan files at `/app/source/<filename>.3mf` from prior failed retries inside an affected user's container can be safely deleted; they were never indexed in the DB, never reachable from the UI, and would have vanished on the next container restart anyway.
- **SpoolBuddy weight sync no longer silently lands on a stale local row when Spoolman is enabled (#1530, reported by @chesterakl)** — Reporter (Spoolman mode, H2C, internal "manually add then NFC-link" flow) saw the SpoolBuddy "Sync Weight" button flip to "Synced!" but the Spoolman-backed inventory listing never updated. Cause: `POST /spoolbuddy/scale/update-spool-weight` (`backend/app/api/routes/spoolbuddy.py`) ran the lookup local-DB-first and only fell through to Spoolman on local miss — but the upstream `nfc/tag-scanned` route is exclusive (always-Spoolman when `spoolman_enabled=true`, after the #1119 / nfc-routing fix). When the user's local DB still held a stale `Spool` row that happened to share a numeric id with the Spoolman spool the NFC tag mapped to, the sync endpoint absorbed the update into the stale local row, returned 200 with the local `weight_used`, and the actual Spoolman spool went untouched. The support log confirms it: 17 sync attempts across two days, every line logged `SpoolBuddy updated spool 2 weight: …g on scale, …g used` (the local-branch log format) and the `SpoolBuddy updated Spoolman spool …` line (which only fires in the Spoolman branch) never appeared. The bug couldn't be reproduced on developer setups because they don't carry a leftover local row with a colliding id. **Fix**: `update_spool_weight` now routes exactly like `nfc_tag_scanned` — `_get_spoolman_client_or_none(db)` first, and that result picks the branch exclusively. Spoolman mode goes straight to Spoolman with no local-DB read; local mode does the local update and returns 404 (not "fallback to Spoolman") on a local miss. Matches [[feedback_inventory_modes_parity]] — the two inventory modes must behave identically from the user's perspective, including which row gets written. The docstring now spells out the routing contract so the next reader doesn't reintroduce the local-first read. **Tests**: 1 new regression test in `TestUpdateSpoolWeightSpoolman.test_stale_local_row_does_not_shadow_spoolman` — creates a local `Spool` with the same numeric id as a mocked Spoolman spool, posts the sync, asserts (a) Spoolman's `update_spool` was called with the correct remaining weight, and (b) the local row's `weight_used` and `last_scale_weight` are unchanged after a `refresh()` against the live DB. The existing 8 tests in that class continue to assert the Spoolman branch math (filament/spool-level tare priority, 404 / 503 mappings, 250g fallback warning). 9/9 green; 126/126 across the spoolbuddy + spoolman-filament-patch integration suites green under `pytest -n 30`. **Cleanup hint for affected users**: anyone in Spoolman mode with leftover local Spool rows from before they switched should delete those rows — they're inert under the new routing, but they were eating sync attempts under the old. Backend ruff clean.
- **Paused prints no longer inflate maintenance hours (#1521, reported by @TempleClause)** — The `track_printer_runtime` background task in `backend/app/main.py` counted both `RUNNING` and `PAUSE` states equally toward `runtime_seconds`, which feeds every hours-based maintenance interval (lubricate rods, clean nozzle, check belts, etc.). Maintenance items measure *mechanical wear*, and pause time involves no motion — so a print paused overnight stretched the maintenance clock forward by ~8 h without any actual wear, triggering "lubricate rods" warnings earlier than warranted. Reporter found this by code review (no support bundle), flagged it cleanly with the exact line in `main.py` and three ranked solution options. **Fix**: option 1 (exclude PAUSE entirely) — `state.state in ("RUNNING", "PAUSE")` → `state.state == "RUNNING"`. PAUSE now follows the same path as FINISH / IDLE / PREPARE: the elapsed-time accumulator skips it, and `last_runtime_update` is cleared so a later RUNNING transition starts fresh and doesn't back-bill the pause. No setting / toggle (reporter's option 3 was deliberately the throwaway — this is a wear-tracking semantic, not a user preference); no cap (option 2) — wear during pause is zero, not "reduced". Docstring and field-comment trail updated across `main.py`, `models/printer.py:23`, and the two `api/routes/maintenance.py` route docstrings that all previously described the field as covering "RUNNING and PAUSE states". **Out of scope**: retroactive backfill of existing `runtime_seconds` values — already-accumulated pause time cannot be split out, only future accumulation is fixed. Users with hours-based maintenance intervals already set will see slower accumulation going forward (the correct outcome), so a previously-near-due item may take longer to ring than under the old behaviour. **Tests**: 3 new in `test_runtime_tracking_pause.py` pinning the new contract — PAUSE does NOT accumulate and clears `last_runtime_update`; RUNNING still accumulates and updates the timestamp; a non-active state (FINISH) clears `last_runtime_update` to prevent back-billing the idle time when the printer next goes RUNNING. The tests drive the actual `track_printer_runtime()` coroutine through a single iteration via patched `asyncio.sleep` against an in-memory SQLite DB, so they catch any regression in the predicate at the call site (not just an extracted helper). Backend ruff clean; targeted 24-test rod/runtime subset all green.
- **Quick Stats: user-cancelled prints now have their own bucket and no longer drag down the Success Rate gauge (#1390 follow-up, reported by @IndividualGhost1905)** — Reporter saw `Total prints: 20 / Success: 18 / Failed: 1` and asked where the 20th print went; the breakdown only showed Successful + Failed, so a cancelled run silently inflated the total without appearing anywhere. The earlier #1390 round had committed a test that *locked in* the bug — `it('uses total_prints as denominator so cancelled/stopped events count')` asserted the gauge should divide by `total_prints`, which lumped user/queue-cancelled jobs in with quality outcomes and conflated user intent with printer performance. **Cause**: `PrintLogEntry.status` has six values in production (`completed`, `failed`, `aborted`, `stopped`, `cancelled`, `skipped`) but the Quick Stats endpoint in `api/routes/archives.py` only counted two — `completed` → Successful, `status == "failed"` → Failed — and used a raw `count(*)` for Total Prints, so the other four statuses ended up in Total without surfacing in any breakdown row. `aborted` was particularly silent: classified as a failure elsewhere in the codebase (`failure_analysis.py`, `main.py:430,1729`) but not counted toward `failed_prints` in stats. **Fix**: three-bucket classification across the whole stats surface, matching how the rest of the codebase already groups these statuses. Quick Stats now returns `successful_prints` (completed), `failed_prints` (failed + aborted — printer-detected quality failures), and a new `cancelled_prints` (stopped + cancelled + skipped — user/queue interruptions). The SuccessRateWidget gauge divides by `successful + failed` only, so cancelling a roll because you changed your mind doesn't ding the printer's success rate — a Cancelled row in the breakdown surfaces the count so it doesn't silently vanish from Total Prints. The Failure Analysis service applies the same denominator change (`failure_rate = failed / (successful + failed)`) to both the headline rate and the per-week trend, so a week with no failures but several cancellations no longer reads as a misleading 0/N. **Schema change is additive-safe**: `ArchiveStats.cancelled_prints` defaults to `0` so any historical fixture validating against the model still parses; the frontend type also defaults the display to `0` when the field is missing. **i18n**: new `stats.cancelled` key with real translations across all 9 locales (de/es/fr/it/ja/pt-BR/zh-CN/zh-TW) per [[feedback_translate_dont_fallback]]; parity script clean at 4994 leaves per locale. **Tests**: existing `it('uses total_prints as denominator …')` test inverted to assert the new behaviour (40 completed / 20 failed / 35 cancelled → gauge shows 67%, Cancelled row reads 35), `cancelled_prints: 0` added to the shared mock so the unchanged-display assertion (140/150 → 93%) still holds since `140 / (140 + 10) = 93.33%` rounds identically. 33 StatsPage tests + 6 backend stats/failure tests green; frontend build + backend ruff clean. **Follow-up (cosmetic):** the new Cancelled row's Ban icon rendered in `text-bambu-gray` while the Successful and Failed icons used semantic `text-status-ok` / `text-status-error` tokens — reporter (@IndividualGhost1905) noted the asymmetry and asked for an orange to match what Archives + notification badges use for cancelled. Switched the Cancelled row to `text-status-warning` (amber-500, same token family as the other two rows), so all three icons are now semantic-token-driven and the new row matches the colour the user already associates with cancelled status elsewhere in the UI.
+51 -18
View File
@@ -3905,6 +3905,51 @@ async def get_project_image(
# =============================================================================
def _resolve_source_3mf_path(archive: PrintArchive, source_filename: str) -> Path:
"""Resolve where to write a source 3MF for ``archive``.
Normal archives nest the source under ``<archive_file_dir>/source/``.
"Fallback" archives (created in main.py when MQTT reports a print start
but Bambuddy never saw the source 3MF — cloud / Handy / pre-existing
SD-card prints) carry ``file_path=""``. Joining that with ``base_dir``
via the ``/`` operator silently yields ``base_dir`` itself, whose parent
is ``base_dir.parent`` — which sent the upload to ``/app/source/`` and
raised a 500 on the final ``relative_to`` (#1531). Fallback archives
now land under ``<base_dir>/archive/no_source/<archive_id>/`` instead,
which stays inside the data volume and remains addressable by every
read site that does ``base_dir / archive.source_3mf_path``.
The resolved directory is asserted to be inside ``base_dir`` even when
``archive.file_path`` is populated, so a row corrupted by an old import
or manual SQL edit fails with a clear 500 instead of writing outside
the data volume.
"""
if archive.file_path:
archive_file = settings.base_dir / archive.file_path
source_dir = archive_file.parent / "source"
else:
source_dir = settings.base_dir / "archive" / "no_source" / str(archive.id)
# Containment check via resolve() — catches absolute file_path, `..`
# traversal, and any other shape that escapes the data volume — but we
# return the *literal* source_dir below. Resolving the returned path
# would canonicalise away a symlinked DATA_DIR (legitimate on TrueNAS /
# QNAP / Synology storage pools, and any `-v /symlink:/app/data`
# mount), which would then make the caller's
# ``source_path.relative_to(settings.base_dir)`` raise because the
# left side is canonical and the right is the symlink path.
try:
source_dir.resolve().relative_to(settings.base_dir.resolve())
except ValueError as exc:
raise HTTPException(
500,
f"Archive {archive.id} resolves to a path outside the data directory; cannot attach source.",
) from exc
source_dir.mkdir(parents=True, exist_ok=True)
return source_dir / source_filename
@router.post("/{archive_id}/source")
async def upload_source_3mf(
archive_id: int,
@@ -3921,11 +3966,9 @@ async def upload_source_3mf(
if not file.filename or not file.filename.endswith(".3mf"):
raise HTTPException(400, "File must be a .3mf file")
# Get archive directory and create source subdirectory
file_path = settings.base_dir / archive.file_path
archive_dir = file_path.parent
source_dir = archive_dir / "source"
source_dir.mkdir(exist_ok=True)
# Save the source 3MF file - preserve original filename, strip directory components
source_filename = _safe_filename(file.filename)
source_path = _resolve_source_3mf_path(archive, source_filename)
# Delete old source file if exists
if archive.source_3mf_path:
@@ -3933,10 +3976,6 @@ async def upload_source_3mf(
if old_source_path.exists():
old_source_path.unlink()
# Save the source 3MF file - preserve original filename, strip directory components
source_filename = _safe_filename(file.filename)
source_path = source_dir / source_filename
content = await file.read()
# #1401: validate zip header on source 3MF uploads too — source files
# are uploaded for reprint and slicing, so an invalid one breaks the
@@ -4128,11 +4167,9 @@ async def upload_source_3mf_by_name(
if not archive:
raise HTTPException(404, f"No archive found matching '{print_name}'")
# Get archive directory and create source subdirectory
file_path = settings.base_dir / archive.file_path
archive_dir = file_path.parent
source_dir = archive_dir / "source"
source_dir.mkdir(exist_ok=True)
# Save the source 3MF file - preserve original filename, strip directory components
source_filename = safe_filename
source_path = _resolve_source_3mf_path(archive, source_filename)
# Delete old source file if exists
if archive.source_3mf_path:
@@ -4140,10 +4177,6 @@ async def upload_source_3mf_by_name(
if old_source_path.exists():
old_source_path.unlink()
# Save the source 3MF file - preserve original filename, strip directory components
source_filename = safe_filename
source_path = source_dir / source_filename
content = await file.read()
# #1401: same zip-header check as the other upload routes — the
# match-by-name endpoint is used by slicer post-processing scripts,
@@ -3,6 +3,8 @@
Tests the full request/response cycle for /api/v1/archives/ endpoints.
"""
from pathlib import Path
import pytest
from httpx import AsyncClient
@@ -1157,3 +1159,143 @@ class TestArchiveF3DEndpoints:
response = await async_client.delete("/api/v1/archives/tags/nonexistent-tag")
assert response.status_code == 200
assert response.json()["affected"] == 0
class TestUploadSourceThreeMF:
"""Regression for #1531: source-3MF upload on fallback archives."""
@staticmethod
def _minimal_3mf_bytes() -> bytes:
"""Smallest valid .3mf — the upload path enforces a zip header check."""
import io
import zipfile
buf = io.BytesIO()
with zipfile.ZipFile(buf, "w") as zf:
zf.writestr("[Content_Types].xml", "<types/>")
return buf.getvalue()
@pytest.mark.asyncio
@pytest.mark.integration
async def test_fallback_archive_source_upload_lands_under_base_dir(
self, async_client: AsyncClient, archive_factory, printer_factory, monkeypatch, tmp_path
):
"""Fallback archive (file_path='') must accept a source upload and store it inside base_dir.
Pre-fix, ``Path(base_dir) / ''`` collapsed to ``base_dir`` and the
``.parent`` walked out of the data volume, sending the file to
``/app/source/...`` and crashing on ``relative_to``.
"""
from backend.app.core.config import settings as app_settings
monkeypatch.setattr(app_settings, "base_dir", tmp_path)
printer = await printer_factory()
archive = await archive_factory(
printer.id,
print_name="Cloud Print",
file_path="", # fallback archive — no source 3MF was archived
filename="Cloud Print.3mf",
)
files = {"file": ("cloud_print.3mf", self._minimal_3mf_bytes(), "application/octet-stream")}
response = await async_client.post(f"/api/v1/archives/{archive.id}/source", files=files)
assert response.status_code == 200, response.text
payload = response.json()
rel = payload["source_3mf_path"]
# Stored as a relative path inside base_dir.
assert not rel.startswith("/"), f"source_3mf_path should be relative, got {rel!r}"
# File physically landed under base_dir (NOT escaped to /app/source/).
assert (tmp_path / rel).is_file()
# Deterministic fallback location keyed off archive id.
assert rel == f"archive/no_source/{archive.id}/cloud_print.3mf"
@pytest.mark.asyncio
@pytest.mark.integration
async def test_normal_archive_source_upload_unchanged(
self, async_client: AsyncClient, archive_factory, printer_factory, monkeypatch, tmp_path
):
"""Normal archive (file_path set) still nests the source under <archive>/source/."""
from backend.app.core.config import settings as app_settings
monkeypatch.setattr(app_settings, "base_dir", tmp_path)
printer = await printer_factory()
# archive_factory's default file_path is "archives/test/test_print.gcode.3mf".
archive = await archive_factory(printer.id, print_name="Real Print")
files = {"file": ("real_print.3mf", self._minimal_3mf_bytes(), "application/octet-stream")}
response = await async_client.post(f"/api/v1/archives/{archive.id}/source", files=files)
assert response.status_code == 200, response.text
rel = response.json()["source_3mf_path"]
assert rel == "archives/test/source/real_print.3mf"
assert (tmp_path / rel).is_file()
@pytest.mark.asyncio
@pytest.mark.integration
async def test_symlinked_data_dir_upload_succeeds(
self, async_client: AsyncClient, archive_factory, printer_factory, monkeypatch, tmp_path
):
"""Regression: DATA_DIR that's a symlink to the real storage must not break the upload.
Common on TrueNAS / Synology / QNAP storage pools, and any
``-v /symlinked/host/path:/app/data`` mount. The helper resolves
only for the containment check and returns literal paths so the
caller's ``relative_to(settings.base_dir)`` doesn't trip over a
canonical-vs-symlink mismatch.
"""
from backend.app.core.config import settings as app_settings
real_dir = tmp_path / "real_storage"
real_dir.mkdir()
symlink_dir = tmp_path / "data_via_symlink"
symlink_dir.symlink_to(real_dir)
monkeypatch.setattr(app_settings, "base_dir", symlink_dir)
printer = await printer_factory()
archive = await archive_factory(
printer.id,
print_name="Symlinked Print",
file_path="archives/X1C/print.gcode.3mf",
filename="print.gcode.3mf",
)
files = {"file": ("print.3mf", self._minimal_3mf_bytes(), "application/octet-stream")}
response = await async_client.post(f"/api/v1/archives/{archive.id}/source", files=files)
assert response.status_code == 200, response.text
rel = response.json()["source_3mf_path"]
assert rel == "archives/X1C/source/print.3mf"
# Reachable via both the symlink and the canonical path.
assert (symlink_dir / rel).is_file()
assert (real_dir / rel).is_file()
@pytest.mark.asyncio
@pytest.mark.integration
async def test_absolute_file_path_rejected_with_clear_500(
self, async_client: AsyncClient, archive_factory, printer_factory, monkeypatch, tmp_path
):
"""A row whose file_path is absolute (corrupted by old import / manual edit)
must fail with the explicit "outside the data directory" message, not silently
write outside base_dir."""
from backend.app.core.config import settings as app_settings
monkeypatch.setattr(app_settings, "base_dir", tmp_path)
printer = await printer_factory()
archive = await archive_factory(
printer.id,
print_name="Corrupt Path",
file_path="/tmp/totally_outside.gcode.3mf",
filename="totally_outside.gcode.3mf",
)
files = {"file": ("totally_outside.3mf", self._minimal_3mf_bytes(), "application/octet-stream")}
response = await async_client.post(f"/api/v1/archives/{archive.id}/source", files=files)
assert response.status_code == 500
assert "outside the data directory" in response.json()["detail"]
# Did not write anything under the bogus /tmp/source/ either.
assert not (Path("/tmp") / "source").exists() or not (Path("/tmp") / "source" / "totally_outside.3mf").exists()