Browse Source

fix(ftp): tolerate transient 426 when file is intact on the printer (#1417 follow-up)

  Previous daily build (1fac0276) tightened the post-STOR voidresp
  handler to fail on any ftplib.Error, stopping Bambuddy from
  sending a print command for a truncated 3MF. Reporter
  (@enjoylifenow on a P2S) then confirmed — after a clean SD-card
  filesystem check, reformat, and power cycle — that v0.2.4.1
  worked on the same hardware. That proves the 426 returned by
  this firmware revision is noise: the TLS data-channel close
  races the 226 confirmation, server reports failure, file is in
  fact on the SD card.

  Reverting wholesale would re-introduce the silent-truncation
  bug from the original fix. Narrow the rule instead: after an
  ftplib.Error from voidresp, run an FTP SIZE against the upload
  path. SIZE matches the local file size → warn and proceed
  (the reporter's case). SIZE mismatch, or SIZE itself raises →
  fail loudly with full diagnostics (the original tightened
  behavior — preserved).

  Applied identically to upload_file() and upload_bytes() so the
  A1-compatibility manual-transfer path is covered.

  Tests: two regressions from the previous round renamed and
  split into intact / truncated / size-check-fails. Intact-file
  tests inject SIZE explicitly because pyftpdlib only flushes on
  a clean voidresp — which can't happen when we monkeypatch
  voidresp to raise. Docstring spells that out. 87 FTP unit tests
  green; 118 FTP-touching tests across unit+integration green;
  ruff clean.

  The View-Timelapse-greyed-out behavior #1417 was originally
  about stays untouched; once the reporter confirms upload
  reliability is back, that diagnosis continues on a healthy
  install.
maziggy 1 week ago
parent
commit
9c934c905d
3 changed files with 160 additions and 33 deletions
  1. 0 0
      CHANGELOG.md
  2. 61 19
      backend/app/services/bambu_ftp.py
  3. 99 14
      backend/tests/unit/services/test_bambu_ftp.py

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


+ 61 - 19
backend/app/services/bambu_ftp.py

@@ -448,18 +448,40 @@ class BambuFTPClient:
                 finally:
                     self._ftp.sock.settimeout(old_timeout)
             except ftplib.Error as e:
-                # The printer's FTP server explicitly told us the transfer
-                # failed (e.g. 426 "Failure reading network stream" seen on
-                # some P2S firmware revisions). The file on the SD card is
-                # truncated — never report success or send a print command
-                # for it. Re-raise so the outer handler returns False.
-                logger.error(
-                    "FTP STOR rejected by printer for %s: %s (%s)",
-                    remote_path,
-                    e,
-                    type(e).__name__,
-                )
-                raise
+                # Some P2S firmware revisions return ftplib.Error (e.g. 426
+                # "Failure reading network stream") on voidresp() even when
+                # the file landed fully on the SD card — the TLS data
+                # channel close races the 226 confirmation (#1417 follow-up).
+                # Verify via SIZE: if the server-side file size matches what
+                # we just uploaded, the file is intact and we proceed with
+                # a warning. If not — or SIZE itself fails — the transfer
+                # was genuinely truncated and we must fail so the print
+                # command doesn't go out for a partial 3MF (the original
+                # reason this catch was tightened in the previous round).
+                try:
+                    server_size = self._ftp.size(remote_path)
+                except (OSError, ftplib.Error) as size_err:
+                    logger.debug("Post-error SIZE check failed: %s", size_err)
+                    server_size = None
+                if server_size is not None and server_size == file_size:
+                    logger.warning(
+                        "FTP STOR returned %s for %s but file is intact on the "
+                        "printer (%s bytes match) — proceeding: %s",
+                        type(e).__name__,
+                        remote_path,
+                        file_size,
+                        e,
+                    )
+                else:
+                    logger.error(
+                        "FTP STOR rejected by printer for %s: %s (%s); server size=%s expected=%s",
+                        remote_path,
+                        e,
+                        type(e).__name__,
+                        server_size,
+                        file_size,
+                    )
+                    raise
             except Exception as e:
                 # Timeout or socket-level error reading 226 — the data was sent
                 # on our side and the printer may still have written the file.
@@ -554,13 +576,33 @@ class BambuFTPClient:
                 finally:
                     self._ftp.sock.settimeout(old_timeout)
             except ftplib.Error as e:
-                logger.error(
-                    "FTP STOR rejected by printer for %s: %s (%s)",
-                    remote_path,
-                    e,
-                    type(e).__name__,
-                )
-                return False
+                # Same SIZE-verify path as upload_file (#1417 follow-up):
+                # tolerate a transient 426 if the bytes are actually on the
+                # printer, fail loudly if they aren't.
+                try:
+                    server_size = self._ftp.size(remote_path)
+                except (OSError, ftplib.Error) as size_err:
+                    logger.debug("Post-error SIZE check failed: %s", size_err)
+                    server_size = None
+                if server_size is not None and server_size == len(data):
+                    logger.warning(
+                        "FTP STOR returned %s for %s but file is intact on the "
+                        "printer (%s bytes match) — proceeding: %s",
+                        type(e).__name__,
+                        remote_path,
+                        len(data),
+                        e,
+                    )
+                else:
+                    logger.error(
+                        "FTP STOR rejected by printer for %s: %s (%s); server size=%s expected=%s",
+                        remote_path,
+                        e,
+                        type(e).__name__,
+                        server_size,
+                        len(data),
+                    )
+                    return False
             except Exception:
                 pass  # Timeout / socket-level — proceed, data was sent.
             return True

+ 99 - 14
backend/tests/unit/services/test_bambu_ftp.py

@@ -386,19 +386,49 @@ class TestUpload:
         assert result is False
         client.disconnect()
 
-    def test_upload_426_data_stream_failure_returns_false(self, ftp_client_factory, ftp_server, tmp_path):
-        """426 'Failure reading network stream' from voidresp() must fail.
-
-        Regression for #1401: P2S firmware 01.02.00.00 (and possibly other
-        Bambu firmware revisions) returns 426 after the data channel closes,
-        indicating the printer received only a partial file. Previously the
-        client logged a warning and returned True, so the dispatcher sent a
-        print command for a truncated 3MF and the printer surfaced a
-        confusing 'unable to parse 3mf file' error. The 426 must instead
-        cause the upload to return False.
+    def test_upload_426_with_intact_file_proceeds(self, ftp_client_factory, ftp_server, tmp_path):
+        """Some P2S firmware revisions return 426 on voidresp() even when the
+        file landed fully (TLS data-channel close races the 226). #1417
+        follow-up — verify via SIZE: when server size matches, proceed with
+        a warning instead of failing the dispatch.
+
+        Pre-#1417 the catch raised unconditionally and the reporter saw 11
+        retries fail in a row even though every upload was actually
+        succeeding on the printer side (v0.2.4.1 worked because the prior
+        proceed-with-warning branch tolerated the noise).
         """
         import ftplib
 
+        local = tmp_path / "test.bin"
+        local.write_bytes(b"data" * 256)  # 1024 bytes
+        client = ftp_client_factory()
+        client.connect()
+
+        def raise_426():
+            raise ftplib.error_temp("426 Failure reading network stream.")
+
+        def fake_size(_path):
+            # Real P2S firmware: voidresp returns 426 but the file IS on
+            # the SD card at its full size. Mock can't reproduce both
+            # halves naturally because pyftpdlib only flushes on a clean
+            # voidresp, so we inject SIZE explicitly to model the
+            # printer-side state the user observes.
+            return 1024
+
+        client._ftp.voidresp = raise_426
+        client._ftp.size = fake_size
+
+        result = client.upload_file(local, "/cache/test.bin")
+        assert result is True, "intact file (SIZE match) tolerates 426 noise"
+        client.disconnect()
+
+    def test_upload_426_with_truncated_file_returns_false(self, ftp_client_factory, ftp_server, tmp_path):
+        """The original #1401 fix is preserved: when SIZE confirms the file
+        isn't on the server at full size (or SIZE itself fails), the upload
+        must fail so the dispatcher doesn't send a print command for a
+        partial 3MF."""
+        import ftplib
+
         local = tmp_path / "test.bin"
         local.write_bytes(b"data" * 256)
         client = ftp_client_factory()
@@ -407,25 +437,80 @@ class TestUpload:
         def raise_426():
             raise ftplib.error_temp("426 Failure reading network stream.")
 
+        # Make SIZE report a smaller value — file is genuinely truncated.
+        def fake_size(_path):
+            return 100
+
         client._ftp.voidresp = raise_426
+        client._ftp.size = fake_size
 
         result = client.upload_file(local, "/cache/test.bin")
-        assert result is False, "Upload must fail on 426 to prevent dispatching a truncated file"
+        assert result is False, "truncated file (SIZE mismatch) must fail"
         client.disconnect()
 
-    def test_upload_bytes_426_data_stream_failure_returns_false(self, ftp_client_factory, ftp_server):
-        """upload_bytes() also fails on 426 (same root cause as upload_file)."""
+    def test_upload_426_with_size_check_failing_returns_false(self, ftp_client_factory, ftp_server, tmp_path):
+        """If SIZE itself fails (e.g. server too broken to answer), assume
+        the worst and fail — better a retry than a print on a partial file.
+        """
         import ftplib
 
+        local = tmp_path / "test.bin"
+        local.write_bytes(b"data" * 256)
         client = ftp_client_factory()
         client.connect()
 
         def raise_426():
             raise ftplib.error_temp("426 Failure reading network stream.")
 
+        def raise_size(_path):
+            raise ftplib.error_perm("550 File not found.")
+
         client._ftp.voidresp = raise_426
+        client._ftp.size = raise_size
+
+        result = client.upload_file(local, "/cache/test.bin")
+        assert result is False
+        client.disconnect()
+
+    def test_upload_bytes_426_with_intact_file_proceeds(self, ftp_client_factory, ftp_server):
+        """upload_bytes() mirrors the same SIZE-verify logic as upload_file."""
+        import ftplib
 
-        result = client.upload_bytes(b"x" * 1024, "/cache/bytes.bin")
+        client = ftp_client_factory()
+        client.connect()
+        data = b"x" * 1024
+
+        def raise_426():
+            raise ftplib.error_temp("426 Failure reading network stream.")
+
+        def fake_size(_path):
+            return 1024  # printer-side file matches expected size
+
+        client._ftp.voidresp = raise_426
+        client._ftp.size = fake_size
+
+        result = client.upload_bytes(data, "/cache/bytes.bin")
+        assert result is True
+        client.disconnect()
+
+    def test_upload_bytes_426_with_truncated_file_returns_false(self, ftp_client_factory, ftp_server):
+        """The truncated branch for upload_bytes()."""
+        import ftplib
+
+        client = ftp_client_factory()
+        client.connect()
+        data = b"x" * 1024
+
+        def raise_426():
+            raise ftplib.error_temp("426 Failure reading network stream.")
+
+        def fake_size(_path):
+            return 100
+
+        client._ftp.voidresp = raise_426
+        client._ftp.size = fake_size
+
+        result = client.upload_bytes(data, "/cache/bytes.bin")
         assert result is False
         client.disconnect()
 

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