diff --git a/CHANGELOG.md b/CHANGELOG.md index 009bc9430..28cdea24a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,8 @@ All notable changes to Bambuddy will be documented in this file. ## [0.2.5b1] - Unreleased ### Fixed +- **FTP passive-port pool now sliced per-VP (10 ports each) so bridge-mode Docker drops from ~3.5 GB to ~210 MB host RAM (#1646, reported by @TheFou — followed up with corrections we acted on)** — Reporter on a Linux Docker VM (`network_mode: host` not viable because other containers already bind the same ports) measured 2002 `docker-proxy` host processes spawned from the previously-exposed `50000-51000:50000-51000` range — one process per port per address family, ~3.5 MB RSS each, ~3.5 GB total that doesn't show up in `docker stats` because it's host-level not container-level. **Root cause: shared port pool, treated as symptom not cause.** `VirtualPrinterFTPServer` exposed `PASSIVE_PORT_MIN/MAX` as **class constants** (`backend/app/services/virtual_printer/ftp_server.py:573-574`), so every VP's FTP session passed the same `(50000, 51000)` range into `_bind_passive_port` and competed on the same 0.0.0.0 binds. The widening from 100 → 1001 ports in an earlier round had been collision-avoidance headroom for multi-VP-on-shared-bind, but the cost was paid by every install — including the reporter's single-VP install that only ever needed ~10 ports of headroom. **Fix: per-VP non-overlapping slices, allocated by VP id.** New module-level `compute_passive_port_slice(vp_id) → (port_min, port_max)` returns a 10-port window: VP id 1 → 50000-50009, VP id 2 → 50010-50019, …, VP id 100 → 50990-50999. Class constants are gone; `VirtualPrinterFTPServer.__init__` now takes `passive_port_min` / `passive_port_max` instance args. `manager.py` computes the slice at server-construction time from `self.id` and passes it in. Result for the reporter (single VP): 10 exposed ports → 20 docker-proxy processes → ~70 MB instead of ~3.5 GB. Three VPs → 30 ports → ~210 MB. **Wrap-around behaviour pinned**: VP ids beyond `PASSIVE_MAX_SLOTS = 100` wrap modulo 100 (an install that's churned through many VPs over time still produces a valid in-range slice). A same-slot collision (vp_id 101 lands on the same slice as vp_id 1) falls back to the per-session 10-attempt random retry that pre-#1646 code already had — same recovery, no regression. **Compose default narrowed**: `docker-compose.yml` now exposes `50000-50029:50000-50029` by default (covers 3 VPs out of the box) instead of the 1001-port range. The comment explains how to widen for more VPs (`50000-500N9` for `N = vp_count - 1`) and that proxy-mode VPs still need `50000-50100:50000-50100` because proxy mode forwards the real printer's full range — that codepath uses a separate `TCPProxy.FTP_DATA_PORT_MIN/MAX` and isn't sliced (the real printer owns that range, not Bambuddy). **Doc corrections in the same drop**: the previous warning over-stated `userland-proxy: false` as "confirmed by the reporter" — TheFou had flagged it as theoretical, not tested; the new comment doesn't push it as a recommendation at all (it's a global daemon flag, too blunt for a per-container problem). The new comment also explicitly names Linux multi-service hosts (NAS, dedicated Docker VMs, Unraid, Synology DSM) as a primary bridge-mode audience instead of leaving the warning under a "macOS/Windows" framing that TheFou pointed out missed his use case. Acknowledges that host-mode default is a deliberate trade-off for SSDP discovery, not a security-blind default. **Tests**: 10 new in `test_vp_ftp_port_slicing.py` — `compute_passive_port_slice` pins: vp_id=1 starts at base, consecutive vp_ids get adjacent non-overlapping slices, no two distinct vp_ids within MAX_SLOTS share a port (exhaustive across all 100 slots), wraps modulo MAX_SLOTS, top slot stays within the documented pool, non-positive vp_ids clamp to slot 0 (defensive — never produce a negative port that would crash `asyncio.start_server`). Two `VirtualPrinterFTPServer` instance tests pin: two instances constructed with different slices stay independent (regression guard against re-introducing class-level state), default-arg construction yields a valid one-slice window. Existing proxy-mode test at `test_virtual_printer.py:2269` (101 ports for `_ftp_data_proxies`) stays green — that path is unchanged. Full 130-test VP suite green. + - **Print Log "User" column now shows the user for prints started from the Queue (#1670, reported by @JmanB52D)** — Reporter on a P2S with auth enabled, Virtual Printer in Queue mode and Auto-dispatch off: a user uploads a `.3mf` to the VP (FTP, anonymous), then logs into Bambuddy and clicks ▶ on the staged queue item to start it; the print finishes and the PrintLogEntry's User column is blank. Same setup with the VP in Archive (slicer-initiated) mode correctly attributes the user. **Root cause: two-link gap on the Queue→manual-start dispatch path.** (a) `POST /queue/{id}/start` (`print_queue.py:1039`) auth-protected, but the route's user dep was bound to `_` and discarded — the clicker was never recorded. (b) `PrintScheduler._start_print` (`print_scheduler.py:1886`) dispatches the queue item directly and never calls `printer_manager.set_current_print_user(...)`. The print-complete callback (`main.py:3513`) reads `_print_user_info = printer_manager.get_current_print_user(printer_id)` — which is only ever populated by `background_dispatch.py:747/943` (the Archive→Print and Library→Print flows). Queue dispatch had no equivalent hop, so `_print_user_info` was always `None` and the PrintLogEntry's `created_by_username` landed `NULL`. **Fix (two-sided):** (1) `print_queue.py /start` now binds the auth dep to `user: User | None` and writes `item.created_by_id = user.id` when `user is not None AND item.created_by_id is None` — credits the clicker on VP-uploaded (unattributed) items without overwriting existing attribution from UI-added queue items (matches the standard "first claim wins" ownership rule in `auth.py::require_ownership_permission`). (2) `print_scheduler.py` gains a small `_propagate_owner_to_printer_manager` helper, called from `_start_print` immediately after `register_expected_print`: when `item.created_by_id` resolves to a real User row, it forwards `(printer_id, owner.id, owner.username)` into `printer_manager.set_current_print_user`. No-ops cleanly when the item has no owner (auto-dispatched VP items intrinsically) or when the user row is missing (e.g. user deleted between queue-add and dispatch — the print log row falls back to un-credited rather than crashing the dispatch). **Tests:** 6 new in `test_queue_start_user_attribution.py` — three route tests pin (a) authenticated `/start` writes `created_by_id` on an unattributed item, (b) an existing owner is preserved when a different user clicks `/start`, (c) auth-disabled leaves `created_by_id=NULL` (no synthetic placeholder user invented); three helper tests pin (d) the propagation forwards the resolved username into `set_current_print_user`, (e) a `None` owner is silently skipped, (f) a missing User row is silently skipped instead of raising. Full 63-test `test_print_queue_api.py` suite stays green. Backend ruff clean. - **AMS drying popover's "Start Drying" button is no longer hidden behind iOS Safari's bottom URL bar on iPhone (#1669, reported via in-app bug report, iPhone 17 Safari)** — Reporter could see the temperature / duration sliders and the "Rotate spool during drying" checkbox but couldn't reach the orange Start Drying button at the bottom of the popover — only a thin sliver of it was visible just above Safari's URL bar. **Root cause:** the popover sizes its `maxHeight` against CSS `100vh` (`PrintersPage.tsx:5443`) and positions itself using `window.innerHeight` (via `computePopoverPosition`, `popoverPosition.ts:53`). On iOS Safari both of those report the **layout** viewport — the full screen ignoring the bottom URL/toolbar overlay — not the visual viewport. The popover therefore extends *behind* Safari's bottom toolbar and the footer button gets clipped. Earlier iterations of the same surface (#1447 popover-off-bottom, #1458 footer-scroll-reachability) fixed desktop / normal-viewport cases but assumed `100vh` matched the visible viewport. **Fix:** two-line change. (a) `frontend/src/pages/PrintersPage.tsx:5443` switches `maxHeight: calc(100vh - …)` → `calc(100dvh - …)` so the dynamic viewport units shrink with iOS toolbars. (b) `frontend/src/utils/popoverPosition.ts:53` defaults `viewportHeight` from `window.visualViewport?.height ?? window.innerHeight` so the flip-above decision also uses the actually-visible area; the existing optional override still wins (tests keep their explicit viewport values). Result: when the iOS toolbar is up, either the popover flips above the trigger earlier (visualViewport too short for below-placement), or the body scrolls within a capped maxHeight and the `shrink-0` footer stays pinned to the visible bottom — the Start Drying button is reachable in both cases. **Tests:** 3 new in `popoverPosition.test.ts::computePopoverPosition (#1669)` — flip-above triggers when visualViewport.height (700) is shorter than innerHeight (800) and the trigger position would only overflow under the visual viewport; falls back to innerHeight when visualViewport is unavailable (older WebViews / jsdom); an explicit `viewportHeight` override still wins over a configured visualViewport.height (test-injection contract). 8 pre-existing tests stay green. dvh / svh browser support — Safari 15.4+, Chrome 108+, Firefox 101+ — comfortably covers iPhone 17 Safari and every supported desktop browser; no behavioural change on non-iOS. diff --git a/backend/app/services/virtual_printer/ftp_server.py b/backend/app/services/virtual_printer/ftp_server.py index 34b976de9..d31b4dc06 100644 --- a/backend/app/services/virtual_printer/ftp_server.py +++ b/backend/app/services/virtual_printer/ftp_server.py @@ -561,17 +561,40 @@ class FTPSession: await self.send(226, "Transfer complete") -class VirtualPrinterFTPServer: - """Implicit FTPS server that accepts uploads from slicers.""" +PASSIVE_PORT_BASE = 50000 +PASSIVE_SLICE_SIZE = 10 +PASSIVE_MAX_SLOTS = 100 - # Passive-mode data port range. Widened from 50000-50100 (101 ports) to - # 50000-51000 (1001 ports) so concurrent transfers across multiple VPs - # — particularly when a VP falls back to bind 0.0.0.0 (manager.py picks - # this when bind_ip is unset) — don't collide. With 101 ports and 10 - # random pick attempts per session, birthday-style collisions hit - # under load; 1001 ports gives multi-VP setups headroom. - PASSIVE_PORT_MIN = 50000 - PASSIVE_PORT_MAX = 51000 + +def compute_passive_port_slice(vp_id: int) -> tuple[int, int]: + """Return the (min, max) passive-mode data port range for VP `vp_id`. + + Each VP gets a unique non-overlapping slice so bridge-mode Docker users + only need to expose `PASSIVE_SLICE_SIZE * ` ports instead of + the full historical 1001-port pool (#1646 — wide pool × Docker's + userland-proxy spawned ~2000 host processes at ~3.5 GB RAM). vp_id is + taken modulo PASSIVE_MAX_SLOTS so installs that have churned through + many VPs over time still produce a valid in-range slice; a same-slot + collision falls back to the existing per-session 10-attempt random + retry, which is the pre-#1646 behaviour and recovers gracefully. + """ + slot = (max(vp_id, 1) - 1) % PASSIVE_MAX_SLOTS + port_min = PASSIVE_PORT_BASE + slot * PASSIVE_SLICE_SIZE + port_max = port_min + PASSIVE_SLICE_SIZE - 1 + return port_min, port_max + + +class VirtualPrinterFTPServer: + """Implicit FTPS server that accepts uploads from slicers. + + Each VP is given a small non-overlapping passive-mode data-port slice + via `passive_port_min/passive_port_max` (typically computed by + `compute_passive_port_slice(vp_id)` at the call site). The slice is + intentionally narrow — 10 ports per VP fits Bambu-style one-passive- + socket-per-upload sessions with safe headroom, and bridge-mode docker + setups only have to expose `N_vps * 10` ports instead of the historical + 1001-port pool (#1646). + """ def __init__( self, @@ -583,6 +606,8 @@ class VirtualPrinterFTPServer: on_file_received: Callable[[Path, str], None] | None = None, bind_address: str = "0.0.0.0", # nosec B104 vp_name: str = "", + passive_port_min: int = PASSIVE_PORT_BASE, + passive_port_max: int = PASSIVE_PORT_BASE + PASSIVE_SLICE_SIZE - 1, ): """Initialize the FTPS server. @@ -595,6 +620,11 @@ class VirtualPrinterFTPServer: on_file_received: Callback when file upload completes (path, source_ip) bind_address: IP address to bind to (default 0.0.0.0) vp_name: Virtual printer name for log identification + passive_port_min: Low end of this VP's passive-mode data port slice + (inclusive). Per-VP slicing eliminates cross-VP collisions on + shared 0.0.0.0 binds without paying for a 1001-port pool (#1646). + passive_port_max: High end of the slice (inclusive). Defaults + produce a 10-port window starting at PASSIVE_PORT_BASE. """ self.upload_dir = upload_dir self.access_code = access_code @@ -604,6 +634,8 @@ class VirtualPrinterFTPServer: self.on_file_received = on_file_received self.bind_address = bind_address self.vp_name = vp_name + self.passive_port_min = passive_port_min + self.passive_port_max = passive_port_max self._server: asyncio.Server | None = None self._running = False # Set after the socket is bound and the server is accepting connections, @@ -673,8 +705,8 @@ class VirtualPrinterFTPServer: logger.info("Implicit FTPS server started on port %s", self.port) logger.info( "FTP passive data port range: %s-%s", - self.PASSIVE_PORT_MIN, - self.PASSIVE_PORT_MAX, + self.passive_port_min, + self.passive_port_max, ) if self._pasv_address: logger.info("FTP PASV address override: %s", self._pasv_address) @@ -707,7 +739,7 @@ class VirtualPrinterFTPServer: access_code=self.access_code, ssl_context=self._ssl_context, on_file_received=self.on_file_received, - passive_port_range=(self.PASSIVE_PORT_MIN, self.PASSIVE_PORT_MAX), + passive_port_range=(self.passive_port_min, self.passive_port_max), pasv_address=self._pasv_address, bind_address=self.bind_address, vp_name=self.vp_name, diff --git a/backend/app/services/virtual_printer/manager.py b/backend/app/services/virtual_printer/manager.py index 1cfc3f40a..f527bcb35 100644 --- a/backend/app/services/virtual_printer/manager.py +++ b/backend/app/services/virtual_printer/manager.py @@ -20,7 +20,7 @@ from backend.app.models.virtual_printer import ( ) from backend.app.services.virtual_printer.bind_server import BindServer from backend.app.services.virtual_printer.certificate import CertificateService -from backend.app.services.virtual_printer.ftp_server import VirtualPrinterFTPServer +from backend.app.services.virtual_printer.ftp_server import VirtualPrinterFTPServer, compute_passive_port_slice from backend.app.services.virtual_printer.mqtt_bridge import MQTTBridge from backend.app.services.virtual_printer.mqtt_server import SimpleMQTTServer from backend.app.services.virtual_printer.ssdp_server import SSDPProxy, VirtualPrinterSSDPServer @@ -749,7 +749,12 @@ class VirtualPrinterInstance: self._tasks = [] - # FTP server + # FTP server. Each VP gets a non-overlapping passive-mode port slice + # derived from its DB id so bridge-mode Docker users only have to + # expose a narrow range (#1646). Default slice is 10 ports per VP; + # see ftp_server.compute_passive_port_slice for the wrap-around + # behaviour on installs with very high VP ids. + passive_port_min, passive_port_max = compute_passive_port_slice(self.id) self._ftp = VirtualPrinterFTPServer( upload_dir=self.upload_dir, access_code=self.access_code, @@ -758,6 +763,8 @@ class VirtualPrinterInstance: on_file_received=self.on_file_received, bind_address=bind_addr, vp_name=self.name, + passive_port_min=passive_port_min, + passive_port_max=passive_port_max, ) self._tasks.append( asyncio.create_task( diff --git a/backend/tests/unit/services/test_vp_ftp_port_slicing.py b/backend/tests/unit/services/test_vp_ftp_port_slicing.py new file mode 100644 index 000000000..4a56e87b7 --- /dev/null +++ b/backend/tests/unit/services/test_vp_ftp_port_slicing.py @@ -0,0 +1,133 @@ +"""Tests for the per-VP FTP passive-port slice helper (#1646). + +Each VP is allocated a non-overlapping 10-port slice from the +PASSIVE_PORT_BASE pool. Bridge-mode Docker users only have to expose +`PASSIVE_SLICE_SIZE * N_vps` ports instead of the historical 1001-port +pool that spawned ~2000 docker-proxy host processes (~3.5 GB RAM). + +Slicing properties pinned here: + - Slice 0 covers PASSIVE_PORT_BASE..+SLICE_SIZE-1 (the only slice that + aligns with the compose file's narrowest default exposure). + - Each subsequent vp_id advances by exactly SLICE_SIZE — no overlap, no + gap. + - vp_ids beyond MAX_SLOTS wrap around (modulo) so installs that have + churned through many VPs over time still produce a valid in-range + slice; same-slot collisions fall back to the per-session 10-attempt + random retry, which is the same behaviour as pre-#1646. + - Defensive: a non-positive vp_id (shouldn't occur, but DBs are + surprising) clamps to slot 0 rather than producing a negative port. +""" + +from __future__ import annotations + +import pytest + +from backend.app.services.virtual_printer.ftp_server import ( + PASSIVE_MAX_SLOTS, + PASSIVE_PORT_BASE, + PASSIVE_SLICE_SIZE, + compute_passive_port_slice, +) + + +class TestComputePassivePortSlice: + def test_vp_id_one_starts_at_base(self): + port_min, port_max = compute_passive_port_slice(1) + assert port_min == PASSIVE_PORT_BASE + assert port_max == PASSIVE_PORT_BASE + PASSIVE_SLICE_SIZE - 1 + + def test_consecutive_vp_ids_get_adjacent_non_overlapping_slices(self): + slice1_min, slice1_max = compute_passive_port_slice(1) + slice2_min, slice2_max = compute_passive_port_slice(2) + slice3_min, slice3_max = compute_passive_port_slice(3) + assert slice1_max + 1 == slice2_min # no gap + assert slice2_max + 1 == slice3_min # no gap + # Slice width matches the constant — no off-by-one. + assert slice1_max - slice1_min + 1 == PASSIVE_SLICE_SIZE + assert slice2_max - slice2_min + 1 == PASSIVE_SLICE_SIZE + assert slice3_max - slice3_min + 1 == PASSIVE_SLICE_SIZE + + def test_no_two_distinct_vp_ids_within_max_slots_share_a_port(self): + seen: dict[int, int] = {} + for vp_id in range(1, PASSIVE_MAX_SLOTS + 1): + lo, hi = compute_passive_port_slice(vp_id) + for port in range(lo, hi + 1): + assert port not in seen, f"VP {vp_id} clashes with VP {seen[port]} on port {port}" + seen[port] = vp_id + + def test_wraps_modulo_max_slots(self): + """VP id past MAX_SLOTS lands on the same slice as its modulo-N + neighbour — the per-session retry handles the rare collision.""" + first = compute_passive_port_slice(1) + wrapped = compute_passive_port_slice(PASSIVE_MAX_SLOTS + 1) + assert wrapped == first + + def test_top_slot_is_within_base_pool(self): + """The last valid slot must stay below PASSIVE_PORT_BASE + + MAX_SLOTS*SLICE_SIZE so the slice never escapes the pool that + the docker-compose comments document for users.""" + _, hi = compute_passive_port_slice(PASSIVE_MAX_SLOTS) + assert hi < PASSIVE_PORT_BASE + PASSIVE_MAX_SLOTS * PASSIVE_SLICE_SIZE + + @pytest.mark.parametrize("bad_id", [0, -1, -999]) + def test_non_positive_ids_clamp_to_slot_zero(self, bad_id): + """Defensive: a bad vp_id from a corrupted row mustn't produce a + negative port and crash asyncio.start_server.""" + assert compute_passive_port_slice(bad_id) == compute_passive_port_slice(1) + + +class TestFTPServerHonoursPerInstanceRange: + """`VirtualPrinterFTPServer` now stores the slice on `self`. Two + instances constructed with different slices must hand each + `FTPSession` the right per-instance range — no class-level leak.""" + + def test_two_instances_independent_ranges(self, tmp_path): + from backend.app.services.virtual_printer.ftp_server import VirtualPrinterFTPServer + + cert = tmp_path / "cert.pem" + cert.write_text("not a real cert") + key = tmp_path / "key.pem" + key.write_text("not a real key") + + a = VirtualPrinterFTPServer( + upload_dir=tmp_path, + access_code="x", + cert_path=cert, + key_path=key, + passive_port_min=50000, + passive_port_max=50009, + ) + b = VirtualPrinterFTPServer( + upload_dir=tmp_path, + access_code="x", + cert_path=cert, + key_path=key, + passive_port_min=50050, + passive_port_max=50059, + ) + + assert (a.passive_port_min, a.passive_port_max) == (50000, 50009) + assert (b.passive_port_min, b.passive_port_max) == (50050, 50059) + # Mutating one must not affect the other (regression guard against + # the pre-fix class-constant layout). + b.passive_port_min = 60000 + assert a.passive_port_min == 50000 + + def test_default_construction_gives_a_one_slice_window(self, tmp_path): + """A consumer that doesn't pass passive_port_min/max should still + get a valid, minimal range — handy for tests and direct callers.""" + from backend.app.services.virtual_printer.ftp_server import VirtualPrinterFTPServer + + cert = tmp_path / "cert.pem" + cert.write_text("x") + key = tmp_path / "key.pem" + key.write_text("x") + + server = VirtualPrinterFTPServer( + upload_dir=tmp_path, + access_code="x", + cert_path=cert, + key_path=key, + ) + assert server.passive_port_min == PASSIVE_PORT_BASE + assert server.passive_port_max - server.passive_port_min + 1 == PASSIVE_SLICE_SIZE diff --git a/docker-compose.yml b/docker-compose.yml index 232d0f35e..85a994dba 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -34,22 +34,31 @@ services: # - "6000:6000" # Virtual printer file transfer tunnel # - "322:322" # Virtual printer RTSP camera (X1/H2/P2; proxy mode + non-proxy modes with a target printer) # - "2024-2026:2024-2026" # Virtual printer proprietary ports (A1/P1S) - # - "50000-51000:50000-51000" # Virtual printer FTP passive data (widened from 50000-50100 for multi-VP headroom) + # - "50000-50029:50000-50029" # Virtual printer FTP passive data (3 VPs × 10-port slice) # - # ⚠️ Bridge-mode + Docker's default userland proxy: the 1001-port FTP - # passive range spawns ~2000 docker-proxy host processes (IPv4+IPv6 - # × 1001 ports), each pinning ~3.5 MB of host RAM, for a ~3.5 GB - # footprint that doesn't show up in `docker stats` because it's - # host-level, not container-level (#1646). Linux's host-mode default - # above sidesteps this entirely. If you genuinely need bridge mode - # (e.g. Docker Desktop on macOS/Windows), set - # { "userland-proxy": false } - # in /etc/docker/daemon.json and restart Docker. Confirmed to clear - # the issue by the reporter; the kernel does NAT directly via - # iptables/nftables, no per-port host process needed. Only side- - # effect is that connections originating from 127.0.0.1 on the host - # itself can't reach the container — fine for nearly every - # Bambuddy install. + # FTP passive-mode port slicing (#1646): non-proxy VPs (Archive / Review / + # Queue modes) get a 10-port slice each, allocated by VP id — VP 1 → + # 50000-50009, VP 2 → 50010-50019, VP 3 → 50020-50029, etc. The default + # exposure above covers 3 VPs; widen the range to cover more + # (`50000-500N9` where N = vp_count - 1). Proxy-mode VPs forward the + # real printer's full 50000-50100 range — if you use proxy mode, expose + # `50000-50100:50000-50100` instead. + # + # Why narrow this matters on bridge mode: with Docker's default + # userland-proxy (true), every exposed port spawns one docker-proxy host + # process per address family (IPv4 + IPv6). The original 1001-port range + # spawned ~2000 such processes, pinning ~3.5 GB of host RAM that doesn't + # appear in `docker stats` (host-level, not container-level). 30 ports + # → ~60 processes → ~210 MB instead. + # + # Bridge mode is the normal setup for any Linux host that runs + # Bambuddy alongside other services (NAS, multi-tenant Docker VM, + # Synology DSM, Unraid) — `network_mode: host` would conflict with + # ports those other services already bind. Bambuddy uses host mode by + # default because SSDP printer discovery needs L2 multicast, but it's + # a deliberate trade-off, not a security-blind default; setups that + # forgo discovery (add printers by IP) can stay on bridge with the + # narrowed range above. volumes: - bambuddy_data:/app/data - bambuddy_logs:/app/logs