mirror of
https://github.com/maziggy/bambuddy.git
synced 2026-10-08 23:21:58 +02:00
fix(printers): recover large-3mf metadata after FTP timeout (#972)
Two-part root cause for missing photos/filament/cost on large prints (#972). The configured ftp_timeout was only plumbed through as the FTP socket timeout; the asyncio.wait_for wrapping run_in_executor stayed on its 60s hardcoded default, so the user's 300s setting never applied. Worse, asyncio.wait_for cannot cancel run_in_executor threads — after the 60s outer timeout fired, the executor thread kept running ftplib.retrbinary and frequently completed the download ~30–60s later, but by then the async wrapper had returned False. with_ftp_retry kept re-attempting the same path, each retry truncating the file the zombie thread had just written, and the archive was ultimately persisted as a fallback with no 3MF. download_file_async now accepts timeout at each call site (plumbed from ftp_timeout) and salvages post-timeout success via an explicit completion flag the executor thread sets only after download_to_file returns True. Per-attempt completion dict so a prot_p zombie can't flip the flag for a later prot_c attempt. A cosmetic // prefix in the directory-search download path is also fixed by replacing string concatenation with posixpath.join.
This commit is contained in:
maziggy
@@ -33,6 +33,7 @@ All notable changes to Bambuddy will be documented in this file.
|
||||
- **Clear Plate Confirmation Bypassed on Power Cycle** ([#961](https://github.com/maziggy/bambuddy/issues/961)) — With Auto Off enabled and another job queued, the smart plug would cut power when a print finished and immediately re-power when the scheduler saw the queue, at which point the printer booted fresh into `IDLE` and the next job auto-dispatched without the "Clear Plate & Start Next" confirmation. Root cause: the plate-cleared gate lived only in the in-memory `PrinterManager._plate_cleared` set, and the scheduler's idle check treated `IDLE` as always-idle regardless of whether a previous finish had been acknowledged — so the gate was lost across both Bambuddy restarts and the IDLE-on-boot state transition. The gate is now an `awaiting_plate_clear` column on the `printers` table, set by `on_print_complete` when a print finishes or fails, cleared by the `/printers/{id}/clear-plate` endpoint and by the scheduler when it dispatches the next job, and rehydrated from the DB into `PrinterManager` on startup. `_is_printer_idle` now short-circuits to not-idle whenever `require_plate_clear` is on and the printer is awaiting ack, regardless of the currently reported state — so the prompt survives Auto Off cycles, Bambuddy restarts, and the printer booting back into `IDLE`. The clear-plate endpoint no longer requires the printer to currently report `FINISH`/`FAILED` (it accepts the ack whenever the awaiting flag is set), and the Printers page widget prompts based on the flag rather than the reported state. Thanks to @miaopas for reporting.
|
||||
- **Insecure Temp File Creation in Backup Export** — The manual backup download endpoint used `tempfile.mktemp()`, which is vulnerable to a symlink race condition (CWE-377). Replaced with `tempfile.mkstemp()` which atomically creates the file, eliminating the TOCTOU window.
|
||||
- **Spoolman Iframe Blocked After 0.2.3b4 Security Headers** — The Spoolman page (Inventory → Spoolman iframe) failed to load when Spoolman was served from the same host as Bambuddy via a reverse proxy. The security-headers middleware added in 0.2.3b4 set `X-Frame-Options: DENY` on every response, which blocked even same-origin iframing. Relaxed to `SAMEORIGIN` so Spoolman (and any other same-origin tool behind the same reverse proxy) can be embedded again, while still preventing cross-origin clickjacking.
|
||||
- **Large 3MF Files Silently Dropped After Print Finish** ([#972](https://github.com/maziggy/bambuddy/issues/972)) — After large prints, the Files tab rows arrived with no thumbnail, no filament breakdown and no cost — the archive row got created as a fallback with no 3MF even when the file was sittable on disk. Two root causes in the 3MF-fetch path. (1) The configured `ftp_timeout` setting (default 30 s, reporter had raised it to 300 s) was only plumbed through as the FTP *socket* timeout; the outer `asyncio.wait_for` wrapping `run_in_executor` was stuck on the hardcoded 60 s default, so the user's 300 s value never applied — every 3MF download was capped at 60 s regardless. (2) `asyncio.wait_for` cannot cancel `run_in_executor` threads: when the 60 s outer timeout fired, the executor thread kept running `ftplib.retrbinary` and frequently completed the download successfully ~30–60 s later — logging `"Successfully downloaded … N bytes"` and caching the working FTP mode — but by then the async wrapper had already returned `False`, so the retry loop kept re-attempting the same path, each attempt truncating the file the zombie thread had just written. After all 4 attempts the wrapper reported `failed after 4 attempts` and the archive was persisted as a fallback (no 3MF, empty `file_path`). The async wrapper now (a) accepts and uses `timeout` at each call site so `ftp_timeout` controls both the asyncio deadline and the socket deadline, and (b) salvages a post-timeout success: when the executor thread has set an explicit completion flag and the file is on disk, the wrapper returns `True` instead of discarding the result. Also fixes a cosmetic `//` prefix in the directory-search download path (`posixpath.join` replaces string concatenation that produced `"//file.3mf"` when the search dir was `"/"`). Thanks to @MartinNYHC for the report and @PurseChicken for the P1S support bundle.
|
||||
- **SD Card Badge Removed** — After four rounds of fixes the printer-card SD status badge still flipped red on H2D when unrelated activity happened on the network (e.g. powering on an A1 caused every H2D to go red simultaneously). The underlying problem is that Bambu firmware SD-state signaling is not reliably derivable from MQTT: the legacy top-level `sdcard` field is only sent on some pushes with inconsistent typing, and `home_flag` bits 8-9 are cleared on heartbeat pushes even when a card is inserted, with no reliable way to distinguish heartbeats from full status reports. The badge has been removed entirely from the Printers page card and the Printer Info modal. Underlying `state.sdcard` parsing is retained (simplified to a plain truthy read of the `sdcard` field only, no more `home_flag` derivation, no heartbeat latches) because the firmware-update precondition check still needs to know whether a card is inserted before starting an update. Thanks to @MartinNYHC for the extensive reporting across all four rounds. Previously, this entry described the H2D badge flap and its three attempted fixes — kept here for history: The original bug toggled between "inserted" (green) and "not inserted" (red) every few seconds on H2D. Root cause: the MQTT parser used a strict identity check (`data["sdcard"] is True`) on the top-level `sdcard` field, but real firmware ships that field inconsistently — bool on some models, int `1`, or a string enum like `"HAS_SDCARD_NORMAL"` on others — so any message carrying a non-bool value flipped the state to `False`. Fixed by deriving the badge from `home_flag` bits 8–9 (`HAS_SDCARD_NORMAL` / `HAS_SDCARD_ABNORMAL`) when present — the canonical firmware source, same as door and store-to-SD parsing — and falling back to a truthy check on the top-level field for firmwares that only send that. Follow-up: the badge was still flapping because Bambu firmwares send partial MQTT pushes that carry the legacy `sdcard` field alone (without `home_flag`), and the fallback was re-engaging on every such push. The parser now latches `home_flag` as the canonical source for the session once seen, so partial pushes carrying only `sdcard` can no longer flip the badge; the latch resets on reconnect so a firmware change still re-learns. Second follow-up: on H2D the badge still showed red on initial Printers-page navigation and flipped to green on reload, because H2D also sends heartbeat-style `home_flag` pushes where bits 8–9 are clear even when a card is inserted. Downgrades from true→false now require three consecutive clear reads (upgrades false→true still apply immediately), so a single heartbeat no longer turns the badge red. Third follow-up: the three-strike counter still lost the race on idle printers — once an A1 or other printer connecting nearby triggered a burst of MQTT activity, idle H2Ds could accumulate ≥3 heartbeat pushes before the next full status report and all flip to red simultaneously. Reworked the derivation: the legacy top-level `sdcard` field is now authoritative when present (truthy check covers bool/int/string firmware variants), `home_flag` bits 8–9 are only consulted on full `push_status` reports (identified by the presence of multiple state markers like `gcode_state`, `mc_percent`, `nozzle_temper`, `print_type`, `stg_cur`, or `ams`), and bare heartbeat pushes carrying `home_flag` alone no longer affect SD state at all. Thanks to @MartinNYHC for reporting.
|
||||
- **CSP Blocked Sidebar Iframes, Service-Worker Registration, and Google Fonts** — The strict `Content-Security-Policy` header added in 0.2.3b4 broke three things at once: (1) custom sidebar links pointing at external HTTPS URLs (e.g. a Grafana/telemetry dashboard) rendered in `ExternalLinkPage` were blocked because no `frame-src` was declared and iframes fell back to `default-src 'self'`; (2) the inline service-worker registration `<script>` at the bottom of `index.html` was blocked by `script-src 'self'`, silently preventing the PWA service worker from installing; (3) the `@import` of Google Fonts' Inter from `index.css` was blocked by `style-src` and `font-src`. Fixed by adding `frame-src 'self' https:` for user-configured HTTPS iframe targets, moving the inline SW-registration script into `/sw-register.js` so `script-src 'self'` covers it without needing `'unsafe-inline'` or per-build hashes, and allowing `https://fonts.googleapis.com` in `style-src` and `https://fonts.gstatic.com` in `font-src`. `frame-ancestors 'none'` is preserved so Bambuddy itself still cannot be framed cross-origin.
|
||||
|
||||
|
||||
+9
-3
@@ -1,5 +1,6 @@
|
||||
import asyncio
|
||||
import logging
|
||||
import posixpath
|
||||
import time
|
||||
from contextlib import asynccontextmanager
|
||||
from datetime import datetime, timedelta, timezone
|
||||
@@ -1741,6 +1742,7 @@ async def on_print_start(printer_id: int, data: dict):
|
||||
printer.access_code,
|
||||
remote_path,
|
||||
temp_path,
|
||||
timeout=ftp_timeout,
|
||||
socket_timeout=ftp_timeout,
|
||||
printer_model=printer.model,
|
||||
max_retries=ftp_retry_count,
|
||||
@@ -1753,6 +1755,7 @@ async def on_print_start(printer_id: int, data: dict):
|
||||
printer.access_code,
|
||||
remote_path,
|
||||
temp_path,
|
||||
timeout=ftp_timeout,
|
||||
socket_timeout=ftp_timeout,
|
||||
printer_model=printer.model,
|
||||
)
|
||||
@@ -1795,25 +1798,28 @@ async def on_print_start(printer_id: int, data: dict):
|
||||
logger.info("Found matching file in %s: %s", search_dir, fname)
|
||||
temp_path = app_settings.archive_dir / "temp" / fname
|
||||
temp_path.parent.mkdir(parents=True, exist_ok=True)
|
||||
remote_full_path = posixpath.join(search_dir, fname)
|
||||
if ftp_retry_enabled:
|
||||
downloaded = await with_ftp_retry(
|
||||
download_file_async,
|
||||
printer.ip_address,
|
||||
printer.access_code,
|
||||
f"{search_dir}/{fname}",
|
||||
remote_full_path,
|
||||
temp_path,
|
||||
timeout=ftp_timeout,
|
||||
socket_timeout=ftp_timeout,
|
||||
printer_model=printer.model,
|
||||
max_retries=ftp_retry_count,
|
||||
retry_delay=ftp_retry_delay,
|
||||
operation_name=f"Download 3MF from {search_dir}/{fname}",
|
||||
operation_name=f"Download 3MF from {remote_full_path}",
|
||||
)
|
||||
else:
|
||||
downloaded = await download_file_async(
|
||||
printer.ip_address,
|
||||
printer.access_code,
|
||||
f"{search_dir}/{fname}",
|
||||
remote_full_path,
|
||||
temp_path,
|
||||
timeout=ftp_timeout,
|
||||
socket_timeout=ftp_timeout,
|
||||
printer_model=printer.model,
|
||||
)
|
||||
|
||||
@@ -623,7 +623,15 @@ async def download_file_async(
|
||||
loop = asyncio.get_event_loop()
|
||||
is_a1 = printer_model in BambuFTPClient.A1_MODELS if printer_model else False
|
||||
|
||||
def _download(force_prot_c: bool = False) -> bool:
|
||||
# Per-attempt completion state: asyncio.wait_for cannot cancel
|
||||
# run_in_executor threads, so on timeout the executor may still complete
|
||||
# the download after we stop waiting. The thread flips `success` to True
|
||||
# ONLY after the file is fully written — a post-timeout check lets us
|
||||
# salvage the download without mistaking an in-progress partial write
|
||||
# for a completed one. Each attempt gets its own dict so a zombie from
|
||||
# an earlier attempt can't flip the flag for a later one.
|
||||
|
||||
def _download(force_prot_c: bool, completion: dict) -> bool:
|
||||
mode_str = "prot_c" if force_prot_c else "prot_p"
|
||||
client = BambuFTPClient(
|
||||
ip_address, access_code, timeout=socket_timeout, printer_model=printer_model, force_prot_c=force_prot_c
|
||||
@@ -632,39 +640,53 @@ async def download_file_async(
|
||||
try:
|
||||
result = client.download_to_file(remote_path, local_path)
|
||||
if result:
|
||||
# Cache the working mode
|
||||
BambuFTPClient.cache_mode(ip_address, mode_str)
|
||||
completion["success"] = True
|
||||
return result
|
||||
finally:
|
||||
client.disconnect()
|
||||
return False
|
||||
|
||||
try:
|
||||
# Check if we have a cached mode for this printer
|
||||
cached_mode = BambuFTPClient._mode_cache.get(ip_address)
|
||||
async def _run(force_prot_c: bool) -> bool:
|
||||
completion = {"success": False}
|
||||
try:
|
||||
return await asyncio.wait_for(
|
||||
loop.run_in_executor(None, lambda: _download(force_prot_c, completion)), timeout=timeout
|
||||
)
|
||||
except TimeoutError:
|
||||
# Give the zombie executor thread a brief moment to finish if it
|
||||
# was already close to done. Only salvage when the thread has
|
||||
# signalled genuine success — checking file size alone would
|
||||
# mistake an in-progress partial write for a completed download.
|
||||
await asyncio.sleep(0.5)
|
||||
if completion["success"] and local_path.exists() and local_path.stat().st_size > 0:
|
||||
logger.info(
|
||||
"FTP download wait_for timed out after %ss for %s, but thread completed (%s bytes) — salvaging",
|
||||
timeout,
|
||||
remote_path,
|
||||
local_path.stat().st_size,
|
||||
)
|
||||
return True
|
||||
logger.warning("FTP download timed out after %ss for %s", timeout, remote_path)
|
||||
return False
|
||||
|
||||
if cached_mode:
|
||||
# Use cached mode
|
||||
force_prot_c = cached_mode == "prot_c"
|
||||
return await asyncio.wait_for(loop.run_in_executor(None, lambda: _download(force_prot_c)), timeout=timeout)
|
||||
# Check if we have a cached mode for this printer
|
||||
cached_mode = BambuFTPClient._mode_cache.get(ip_address)
|
||||
|
||||
# No cached mode - try prot_p first
|
||||
result = await asyncio.wait_for(loop.run_in_executor(None, lambda: _download(False)), timeout=timeout)
|
||||
if cached_mode:
|
||||
force_prot_c = cached_mode == "prot_c"
|
||||
return await _run(force_prot_c)
|
||||
|
||||
if result:
|
||||
return True
|
||||
# No cached mode - try prot_p first
|
||||
if await _run(False):
|
||||
return True
|
||||
|
||||
# Download failed - for A1 models, try prot_c fallback
|
||||
if is_a1:
|
||||
logger.info("FTP download failed with prot_p for A1 model, trying prot_c fallback...")
|
||||
result = await asyncio.wait_for(loop.run_in_executor(None, lambda: _download(True)), timeout=timeout)
|
||||
return result
|
||||
# Download failed - for A1 models, try prot_c fallback
|
||||
if is_a1:
|
||||
logger.info("FTP download failed with prot_p for A1 model, trying prot_c fallback...")
|
||||
return await _run(True)
|
||||
|
||||
return False
|
||||
|
||||
except TimeoutError:
|
||||
logger.warning("FTP download timed out after %ss for %s", timeout, remote_path)
|
||||
return False
|
||||
return False
|
||||
|
||||
|
||||
async def download_file_try_paths_async(
|
||||
|
||||
@@ -702,6 +702,112 @@ class TestAsyncWrappers:
|
||||
)
|
||||
assert result is True
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_download_file_async_timeout_salvages_completed_zombie(self, tmp_path, monkeypatch):
|
||||
"""Executor thread that completes after wait_for timeout is salvaged.
|
||||
|
||||
asyncio.wait_for cannot cancel run_in_executor threads, so the FTP
|
||||
download may still complete after we give up waiting. If the thread
|
||||
genuinely finished (signalled via completion["success"] and the file
|
||||
is on disk), download_file_async should return True rather than False.
|
||||
|
||||
Regression for #972: A1 user with 14 MB 3MF hit the hardcoded 60s
|
||||
timeout, but the download thread finished ~45s later. The successful
|
||||
file was written to disk but the async wrapper returned False, so the
|
||||
archive was created as a fallback with no 3MF data.
|
||||
"""
|
||||
from backend.app.services import bambu_ftp
|
||||
|
||||
# Clear mode cache so prot_p path is exercised.
|
||||
bambu_ftp.BambuFTPClient._mode_cache.pop("127.0.0.1", None)
|
||||
|
||||
local = tmp_path / "zombie.bin"
|
||||
expected_content = b"late arrival but complete"
|
||||
|
||||
class FakeClient:
|
||||
"""Connects instantly, download_to_file sleeps past wait_for's
|
||||
timeout then writes the file and returns True."""
|
||||
|
||||
def __init__(self, *args, **kwargs):
|
||||
pass
|
||||
|
||||
def connect(self):
|
||||
return True
|
||||
|
||||
def download_to_file(self, remote_path, local_path):
|
||||
time.sleep(0.4) # longer than wait_for timeout=0.1
|
||||
local_path.write_bytes(expected_content)
|
||||
return True
|
||||
|
||||
def disconnect(self):
|
||||
pass
|
||||
|
||||
monkeypatch.setattr(bambu_ftp, "BambuFTPClient", FakeClient)
|
||||
monkeypatch.setattr(FakeClient, "_mode_cache", {}, raising=False)
|
||||
monkeypatch.setattr(FakeClient, "A1_MODELS", {"A1"}, raising=False)
|
||||
|
||||
def _noop_cache(ip, mode):
|
||||
pass
|
||||
|
||||
monkeypatch.setattr(FakeClient, "cache_mode", staticmethod(_noop_cache), raising=False)
|
||||
|
||||
result = await download_file_async(
|
||||
"127.0.0.1",
|
||||
"12345678",
|
||||
"/cache/zombie.bin",
|
||||
local,
|
||||
timeout=0.1,
|
||||
printer_model="X1C",
|
||||
)
|
||||
assert result is True
|
||||
assert local.read_bytes() == expected_content
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_download_file_async_timeout_no_salvage_when_incomplete(self, tmp_path, monkeypatch):
|
||||
"""Timeout returns False when thread has not signalled success.
|
||||
|
||||
A partial file on disk (mid-retrbinary) must NOT be mistaken for a
|
||||
completed download — only the thread's explicit success flag permits
|
||||
salvage.
|
||||
"""
|
||||
from backend.app.services import bambu_ftp
|
||||
|
||||
bambu_ftp.BambuFTPClient._mode_cache.pop("127.0.0.1", None)
|
||||
|
||||
local = tmp_path / "partial.bin"
|
||||
|
||||
class FakeClient:
|
||||
def __init__(self, *args, **kwargs):
|
||||
pass
|
||||
|
||||
def connect(self):
|
||||
return True
|
||||
|
||||
def download_to_file(self, remote_path, local_path):
|
||||
# Simulate an in-progress partial write that never completes
|
||||
# within the salvage grace period.
|
||||
local_path.write_bytes(b"partial...")
|
||||
time.sleep(2.0)
|
||||
return True # would complete eventually, but too late
|
||||
|
||||
def disconnect(self):
|
||||
pass
|
||||
|
||||
monkeypatch.setattr(bambu_ftp, "BambuFTPClient", FakeClient)
|
||||
monkeypatch.setattr(FakeClient, "_mode_cache", {}, raising=False)
|
||||
monkeypatch.setattr(FakeClient, "A1_MODELS", set(), raising=False)
|
||||
monkeypatch.setattr(FakeClient, "cache_mode", staticmethod(lambda ip, mode: None), raising=False)
|
||||
|
||||
result = await download_file_async(
|
||||
"127.0.0.1",
|
||||
"12345678",
|
||||
"/cache/partial.bin",
|
||||
local,
|
||||
timeout=0.1,
|
||||
printer_model="X1C",
|
||||
)
|
||||
assert result is False
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_download_file_try_paths_first_succeeds(self, patch_ftp_port, tmp_path):
|
||||
"""download_file_try_paths_async succeeds on first path."""
|
||||
|
||||
Reference in New Issue
Block a user