Sfoglia il codice sorgente

fix(vp): slice FTP passive ports per VP, drop bridge-mode RAM by 95% (#1646)

  The 50000-51000 docker-compose port range spawned ~2000 docker-proxy
  host processes (~3.5 GB RSS) under Docker's default userland-proxy.
  The 1001-port pool was symptom treatment — collisions only matter for
  multi-VP-on-shared-bind, but the cost was paid by every install.

  Each VP now gets a non-overlapping 10-port slice computed from its id
  (VP 1 -> 50000-50009, VP 2 -> 50010-50019, ...). Class constants are
  gone; VirtualPrinterFTPServer takes passive_port_min/max instance args.
  Wraps modulo PASSIVE_MAX_SLOTS = 100, with the existing 10-attempt
  random retry as same-slot collision fallback.

  Compose default narrowed to 50000-50029 (3 VPs). Proxy-mode VPs forward
  the real printer's full range and stay on the separate TCPProxy
  constants. Compose comment rewritten to acknowledge Linux multi-service
  hosts as a primary bridge-mode audience and drop an over-stated
  "confirmed by reporter" claim about userland-proxy=false.
maziggy 3 mesi fa
parent
commit
d597d36d5b

File diff suppressed because it is too large
+ 0 - 0
CHANGELOG.md


+ 44 - 12
backend/app/services/virtual_printer/ftp_server.py

@@ -561,17 +561,40 @@ class FTPSession:
         await self.send(226, "Transfer complete")
         await self.send(226, "Transfer complete")
 
 
 
 
+PASSIVE_PORT_BASE = 50000
+PASSIVE_SLICE_SIZE = 10
+PASSIVE_MAX_SLOTS = 100
+
+
+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 * <vp count>` 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:
 class VirtualPrinterFTPServer:
-    """Implicit FTPS server that accepts uploads from slicers."""
+    """Implicit FTPS server that accepts uploads from slicers.
 
 
-    # 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
+    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__(
     def __init__(
         self,
         self,
@@ -583,6 +606,8 @@ class VirtualPrinterFTPServer:
         on_file_received: Callable[[Path, str], None] | None = None,
         on_file_received: Callable[[Path, str], None] | None = None,
         bind_address: str = "0.0.0.0",  # nosec B104
         bind_address: str = "0.0.0.0",  # nosec B104
         vp_name: str = "",
         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.
         """Initialize the FTPS server.
 
 
@@ -595,6 +620,11 @@ class VirtualPrinterFTPServer:
             on_file_received: Callback when file upload completes (path, source_ip)
             on_file_received: Callback when file upload completes (path, source_ip)
             bind_address: IP address to bind to (default 0.0.0.0)
             bind_address: IP address to bind to (default 0.0.0.0)
             vp_name: Virtual printer name for log identification
             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.upload_dir = upload_dir
         self.access_code = access_code
         self.access_code = access_code
@@ -604,6 +634,8 @@ class VirtualPrinterFTPServer:
         self.on_file_received = on_file_received
         self.on_file_received = on_file_received
         self.bind_address = bind_address
         self.bind_address = bind_address
         self.vp_name = vp_name
         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._server: asyncio.Server | None = None
         self._running = False
         self._running = False
         # Set after the socket is bound and the server is accepting connections,
         # 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("Implicit FTPS server started on port %s", self.port)
             logger.info(
             logger.info(
                 "FTP passive data port range: %s-%s",
                 "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:
             if self._pasv_address:
                 logger.info("FTP PASV address override: %s", self._pasv_address)
                 logger.info("FTP PASV address override: %s", self._pasv_address)
@@ -707,7 +739,7 @@ class VirtualPrinterFTPServer:
             access_code=self.access_code,
             access_code=self.access_code,
             ssl_context=self._ssl_context,
             ssl_context=self._ssl_context,
             on_file_received=self.on_file_received,
             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,
             pasv_address=self._pasv_address,
             bind_address=self.bind_address,
             bind_address=self.bind_address,
             vp_name=self.vp_name,
             vp_name=self.vp_name,

+ 9 - 2
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.bind_server import BindServer
 from backend.app.services.virtual_printer.certificate import CertificateService
 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_bridge import MQTTBridge
 from backend.app.services.virtual_printer.mqtt_server import SimpleMQTTServer
 from backend.app.services.virtual_printer.mqtt_server import SimpleMQTTServer
 from backend.app.services.virtual_printer.ssdp_server import SSDPProxy, VirtualPrinterSSDPServer
 from backend.app.services.virtual_printer.ssdp_server import SSDPProxy, VirtualPrinterSSDPServer
@@ -749,7 +749,12 @@ class VirtualPrinterInstance:
 
 
         self._tasks = []
         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(
         self._ftp = VirtualPrinterFTPServer(
             upload_dir=self.upload_dir,
             upload_dir=self.upload_dir,
             access_code=self.access_code,
             access_code=self.access_code,
@@ -758,6 +763,8 @@ class VirtualPrinterInstance:
             on_file_received=self.on_file_received,
             on_file_received=self.on_file_received,
             bind_address=bind_addr,
             bind_address=bind_addr,
             vp_name=self.name,
             vp_name=self.name,
+            passive_port_min=passive_port_min,
+            passive_port_max=passive_port_max,
         )
         )
         self._tasks.append(
         self._tasks.append(
             asyncio.create_task(
             asyncio.create_task(

+ 133 - 0
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

+ 24 - 15
docker-compose.yml

@@ -34,22 +34,31 @@ services:
     #  - "6000:6000"                  # Virtual printer file transfer tunnel
     #  - "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)
     #  - "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)
     #  - "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:
     volumes:
       - bambuddy_data:/app/data
       - bambuddy_data:/app/data
       - bambuddy_logs:/app/logs
       - bambuddy_logs:/app/logs

Some files were not shown because too many files changed in this diff