diff --git a/CHANGELOG.md b/CHANGELOG.md index da02b43ae..82763943a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ All notable changes to Bambuddy will be documented in this file. ## [0.2.4b1] - Unreleased ### Fixed +- **FTP Download Zombie-Thread Race on Slow WiFi** ([#1014](https://github.com/maziggy/bambuddy/issues/1014)) — Users on 2.4 GHz WiFi with heavy neighborhood interference saw "Successfully downloaded" log lines for queued prints that Bambuddy nonetheless reported as failed, and the slicer file landed in `/app/data/archives/temp/` with the File Manager unable to find it. Root cause: `download_file_async` wrapped the blocking FTP `RETR` in `asyncio.wait_for` with a 30–60 s timeout (user-configurable via `ftp_timeout`), but the wrapped thread couldn't be cancelled. On a slow link the download would overshoot the timeout by 15–30 s, at which point `_run()` waited a hard-coded 0.5 s for the zombie to finish, gave up, and returned failure — which triggered `with_ftp_retry` attempt 2, whose `_download` spawned a brand-new FTP session that contended with attempt 1's still-running transfer. Attempt 1's zombie eventually completed and wrote the file to disk, but by then attempt 2 (and 3, 4) had long since run out their own timeouts with their own fresh `completion` dicts and reported failure; the archive pipeline saw only the final `None` from `with_ftp_retry` and created a fallback archive row with no 3MF data, which is why Skip-Object couldn't find the plate's objects even though the 3MF was on disk. Two fixes: the 0.5 s post-timeout sleep is replaced with a `threading.Event` the worker sets in its `finally` block, and `_run()` waits for that event with a bounded grace of `max(min(ftp_timeout, 30), 0.5)` s — covering the slow-WiFi overshoot case without extending a genuinely stuck connection indefinitely. The log line now includes the grace window (`timed out after Xs (plus Ys grace)`). Regression test `test_download_file_async_timeout_waits_for_slow_zombie` simulates a 1.5 s zombie with a 1.0 s wait_for timeout; old 0.5 s sleep would give up, new 1.0 s grace salvages. The existing `test_download_file_async_timeout_no_salvage_when_incomplete` still passes — a thread that never completes within the grace window still returns failure. Thanks to @heffe2001 for the detailed reproduction and support logs. - **Obico: Cold-Start Capture Timeout Sticks in Status Banner** ([#172](https://github.com/maziggy/bambuddy/issues/172)) — On the very first detection poll after a restart, the initial RTSP snapshot capture occasionally exceeded the 20 s `SNAPSHOT_CAPTURE_TIMEOUT` (the first keyframe from the printer's camera can take a while on a cold RTSP connection). Subsequent polls every ~8 s recovered and captured in ~1.2 s, but the red `× Failed to capture snapshot for printer N` banner in Settings → Failure Detection → Status stayed up forever because `ObicoDetectionService._last_error` was written on failure and never cleared on the next successful poll. The successful branch in `_check_printer` now clears `_last_error` to `None` once a capture + ML call + classification complete, so the banner reflects only errors from recent cycles. Configuration-level errors (missing `external_url`, missing `ml_url`) still persist because they return before the clearing line — users still see them until they fix the setting. Regression test covers: seed `_last_error`, run one successful `_check_printer`, assert `_last_error is None`. Thanks to @fblix for the reproduction and screenshot. - **Printer Card Controls Row Overflows in Chrome** — At Medium card size on a wide viewport, the printer-card controls row (fan badges, airduct mode, print speed, bed jog, then Stop / Pause on the right) visibly overlapped in Chrome while rendering fine in Firefox and Safari. The controls-row layout had a `max-[550px]:flex-wrap` rule on the left badge group that only fires below 550 **viewport** pixels, so on a wide viewport with a narrow card the left group never wrapped — and since its badges don't truncate, Chrome painted the overflowing speed/bed-jog badges on top of the right-pinned Stop/Pause buttons. German locales made it obvious ("Pausieren" is 9 characters). The left group now uses unconditional `flex-wrap`, so when badges don't all fit on one line they wrap inside the left cell instead of colliding with the right cell; the parent row also wraps `gap-y` so Stop/Pause drops to a new line in the worst case. Pre-existing (commit `4ff3e2a6`, Feb 2026), surfaced while testing #939. - **MQTT Smart Plug Subscription Lost After Every Restart** ([#1010](https://github.com/maziggy/bambuddy/issues/1010)) — Users integrating a Shelly (or any other) plug through an external MQTT broker (e.g. ioBroker, Zigbee2MQTT, Home Assistant's MQTT broker) saw the plug's power / state / energy readings go dark after every Bambuddy restart, and the only fix was to open Settings → Smart Plugs, rename the topic to a dummy value, save, rename it back and save again. Root cause: the startup restore path in `main.py` (~line 4120) still used the legacy single-topic model (`mqtt_topic` plus `*_path` kwargs), while the Settings UI save path had been upgraded to the newer per-type model (`mqtt_power_topic` / `mqtt_energy_topic` / `mqtt_state_topic` each with their own paths, multipliers and `mqtt_state_on_value`). Plugs configured entirely with the new per-type fields got skipped at startup because the `if plug.mqtt_topic:` guard short-circuited — which is exactly what a Shelly-via-ioBroker setup looks like, since those publish power and state on separate topics. The "rename, save, rename back" workaround triggered the update endpoint, which was using the correct per-type code and re-established the subscription. Fix: extracted the topic-resolution + `service.subscribe()` call into a single `subscribe_plug_to_mqtt(service, plug)` helper in `backend/app/services/mqtt_smart_plug.py` that preserves legacy fallback, and routed the startup restore, create, and update routes all through it so future schema changes can't cause the three paths to drift again. Regression tests cover: per-type topics restored without a legacy topic set, legacy single-topic backward compat, per-type multipliers overriding legacy, per-type winning when both are set, the empty-config skip case, and topic-list de-duplication. Thanks to @saint-hh for the clear repro steps. diff --git a/backend/app/services/bambu_ftp.py b/backend/app/services/bambu_ftp.py index cc60f004b..fba2bd38e 100644 --- a/backend/app/services/bambu_ftp.py +++ b/backend/app/services/bambu_ftp.py @@ -4,6 +4,7 @@ import logging import os import socket import ssl +import threading import time from collections.abc import Awaitable, Callable from ftplib import FTP, FTP_TLS # nosec B402 @@ -719,46 +720,68 @@ async def download_file_async( # 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. + # for a completed one. Each attempt gets its own dict and event so a + # zombie from an earlier attempt can't flip the flag for a later one. + # The event is set in `_download`'s finally block so the post-timeout + # path can wait for genuine thread completion instead of a fixed sleep. - def _download(force_prot_c: bool, completion: dict) -> bool: + def _download(force_prot_c: bool, completion: dict, done: threading.Event) -> 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 - ) - if client.connect(): - try: - result = client.download_to_file(remote_path, local_path) - if result: - BambuFTPClient.cache_mode(ip_address, mode_str) - completion["success"] = True - return result - finally: - client.disconnect() - return False + try: + client = BambuFTPClient( + ip_address, + access_code, + timeout=socket_timeout, + printer_model=printer_model, + force_prot_c=force_prot_c, + ) + if client.connect(): + try: + result = client.download_to_file(remote_path, local_path) + if result: + BambuFTPClient.cache_mode(ip_address, mode_str) + completion["success"] = True + return result + finally: + client.disconnect() + return False + finally: + done.set() async def _run(force_prot_c: bool) -> bool: completion = {"success": False} + done = threading.Event() try: return await asyncio.wait_for( - loop.run_in_executor(None, lambda: _download(force_prot_c, completion)), timeout=timeout + loop.run_in_executor(None, _download, force_prot_c, completion, done), 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) + # Slow WiFi links commonly overshoot ftp_timeout by 10–30 s without + # actually being stuck, so starting attempt 2 now would just contend + # with the still-progressing RETR on attempt 1 and produce the + # zombie-write race reported in #1014 (file landed on disk minutes + # after the retry loop had already given up). Wait for the worker + # thread to genuinely finish — capped at 30 s so a truly stuck + # connection can't stall a whole attempt indefinitely, with a 0.5 s + # floor so artificially small test timeouts still give zombies a + # realistic window to finish. + grace = max(min(timeout, 30.0), 0.5) + await loop.run_in_executor(None, done.wait, grace) 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", + "FTP download wait_for timed out after %ss for %s, but thread completed within %ss grace (%s bytes) — salvaging", timeout, remote_path, + grace, local_path.stat().st_size, ) return True - logger.warning("FTP download timed out after %ss for %s", timeout, remote_path) + logger.warning( + "FTP download timed out after %ss (plus %ss grace) for %s", + timeout, + grace, + remote_path, + ) return False # Check if we have a cached mode for this printer diff --git a/backend/tests/unit/services/test_bambu_ftp.py b/backend/tests/unit/services/test_bambu_ftp.py index 4e8e8ca5e..d5c7567af 100644 --- a/backend/tests/unit/services/test_bambu_ftp.py +++ b/backend/tests/unit/services/test_bambu_ftp.py @@ -819,6 +819,62 @@ class TestAsyncWrappers: ) assert result is False + @pytest.mark.asyncio + async def test_download_file_async_timeout_waits_for_slow_zombie(self, tmp_path, monkeypatch): + """A zombie that completes within the 30s grace window is salvaged. + + Regression for #1014: on slow WiFi, download_to_file can overshoot the + user's ftp_timeout by 10–30 s without being stuck. The old fixed 0.5 s + post-timeout sleep was too short — it gave up and started attempt 2 + while attempt 1's zombie thread kept running, and by the time the zombie + wrote the file to disk with a success flag, attempt 2 had already + reported failure (its own completion dict was still False). The async + wrapper now waits up to min(timeout, 30 s) for the worker thread to + finish before returning, so a slow-but-progressing download salvages. + """ + from backend.app.services import bambu_ftp + + bambu_ftp.BambuFTPClient._mode_cache.pop("127.0.0.1", None) + + local = tmp_path / "slow_zombie.bin" + expected_content = b"finished during grace window" + + class FakeClient: + """Mimics a slow FTP: wait_for gives up at 1.0 s but RETR takes + 1.5 s total. Old 0.5 s fixed sleep would have bailed (0.5 < 0.5 + extra); new grace = max(min(1.0, 30), 0.5) = 1.0 s covers the + remaining 0.5 s so salvage succeeds.""" + + def __init__(self, *args, **kwargs): + pass + + def connect(self): + return True + + def download_to_file(self, remote_path, local_path): + time.sleep(1.5) # wait_for times out at 1.0 s; zombie finishes 0.5 s later + 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", 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/slow_zombie.bin", + local, + timeout=1.0, + printer_model="X1C", + ) + assert result is True + assert local.read_bytes() == expected_content + @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."""