From 9c934c905d10033ce02897447160fbfdbf55766d Mon Sep 17 00:00:00 2001 From: maziggy Date: Tue, 19 May 2026 11:32:22 +0200 Subject: [PATCH] fix(ftp): tolerate transient 426 when file is intact on the printer (#1417 follow-up) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- CHANGELOG.md | 2 + backend/app/services/bambu_ftp.py | 80 ++++++++++--- backend/tests/unit/services/test_bambu_ftp.py | 113 +++++++++++++++--- 3 files changed, 162 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 564cb9409..3a1e6b374 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,8 @@ All notable changes to Bambuddy will be documented in this file. - **Camera: in-app diagnostic for "Connection lost" (#1395 follow-up)** — Second step of the camera architecture overhaul. When the camera viewer hits its error state, a new **Diagnose** button next to **Retry** runs a staged check against the printer and renders the result inline: which stage failed, how long it took, and a translated remediation hint. Cuts off the "user opens a 'camera broken' ticket → wait days → ask for the support bundle → finally figure out it was their reverse proxy / LAN-only toggle / wrong access code" loop at the user's screen. **Backend** ships `backend/app/services/camera_diagnose.py` (orchestrator) and a new `POST /printers/{id}/camera/diagnose` route in `camera.py`. Stages: (1) `tcp_reachable` — opens a TCP socket to the camera port (322 RTSPS / 6000 chamber image) with a 3-second timeout; distinguishes timeout (`tcp_timeout` → "printer not reachable, check IP/network/power") from refused (`tcp_refused` → "camera port closed, check LAN-only and developer mode") from host-unreachable (`tcp_unreachable` → "printer not reachable"). (2) `first_frame` — captures one JPEG end-to-end via the existing `capture_camera_frame_bytes` pipeline (15-second timeout, same code that powers `/camera/snapshot`); auth, RTSP handshake, and first keyframe collapse into one stage because the user-facing answer is the same regardless of which sub-layer failed. **Live-stream shortcut**: when a viewer is currently watching the printer's camera AND the buffered last-frame timestamp is fresher than 10 seconds, the diagnostic skips the real test and returns `live_stream_active_healthy` — opening a fresh socket would kick the live viewer off on single-camera-connection firmwares (the #1348 reconnect-storm trigger), so we trust the real-world evidence instead. Response includes structured metadata for support triage: `protocol` (rtsp / chamber_image), `port`, `profile` (`default` or the model name with an override — currently only `P2S`), per-stage duration in ms, and the machine-readable summary code. **Frontend** adds `CameraDiagnoseModal.tsx` that fires the API call on mount, renders one row per stage with green-check / red-X / grey-skipped icons, and shows the summary remediation message in a bordered banner styled by overall status. The metadata line at the bottom (protocol / port / profile) lets support triage ask "what does your modal say?" instead of "send the support bundle". A **Run again** button re-runs the diagnostic without dismissing the modal. **EmbeddedCameraViewer** error state grows the Diagnose button (kept "Retry" as the primary action; Diagnose is the escape hatch for users who can't see what's wrong). A small stethoscope icon also lives in the viewer's always-visible control bar between **Refresh** and **Fullscreen**, so pre-flight testing ("did my firmware update break the camera?", "is the camera up before I send a print?") doesn't require waiting for the stream to fail first. Also lifted the previously-hard-coded "Camera unavailable" / "Retry" strings into `camera.unavailable` / `camera.retry` so the error UI is properly translated alongside the new keys. **i18n**: 16 new keys (`unavailable`, `retry`, plus `diagnose.{button,modalTitle,running,runFailed,retry,stage.*,summary.*,meta.*}`) translated across all 8 locales (en/de/fr/it/ja/pt-BR/zh-CN/zh-TW). German "Diagnose" is a real cognate — added to `IDENTICAL_TO_EN_ALLOWED.de` rather than translated to a synthetic. Parity check holds at 4849 leaves per locale. **Tests**: 11 backend unit tests in `test_camera_diagnose.py` cover the live-stream shortcut (skip when fresh, run when stale), the three TCP failure modes (timeout / refused / OSError) → distinct summary codes, the first-frame stage (no-frame and capture-exception cases), the all-OK path, and the result metadata (P2S → P2S profile / rtsp / 322; A1 → default / chamber_image / 6000; X1C → default / rtsp / 322). 1 backend integration test pins the route's response shape end-to-end. 3 frontend tests in `CameraDiagnoseModal.test.tsx` (mounted → API call, failure → translated remediation, Run again → re-call). 5021 backend tests + 1905 frontend tests green; ruff clean; build clean; i18n parity clean. ### Fixed +- **FTP: tolerate transient 426 from buggy printer FTP when the file is actually on the SD card (#1417 follow-up, reported by @enjoylifenow)** — In the previous daily build, commit `1fac0276` tightened the post-STOR confirmation handler in `bambu_ftp.py` so that any `ftplib.Error` from `voidresp()` (including `error_temp 426` "Failure reading network stream") would fail the upload outright. The goal was to stop Bambuddy from sending a print command for a truncated 3MF when the printer's FTP server explicitly told us the data stream was cut — exactly the scenario surfacing the user's earlier "unable to parse 3mf file" 30 s into a print. Reporter then confirmed (after running a filesystem check + reformat + power cycle, all clean) that the same install **worked fine on v0.2.4.1** — proving that for the specific P2S firmware revision in question, the 426 is *noise*: the TLS data-channel close races the 226 confirmation, the server reports failure on voidresp, but the file *did* land fully on the SD card. The previous proceed-with-warning behaviour was accidentally correct for that firmware quirk. Reverting wholesale would re-introduce the silent-truncation bug, so instead narrow the rule: when voidresp raises an `ftplib.Error`, immediately follow up with an FTP `SIZE` query against the freshly-uploaded path. If the server-side size matches what Bambuddy just sent, the file is provably intact and Bambuddy proceeds with a warning (`FTP STOR returned error_temp for X but file is intact on the printer (N bytes match) — proceeding`). If the size doesn't match — or `SIZE` itself raises — the transfer was genuinely truncated (or the server is in too broken a state to be trusted) and the upload fails loudly with the error log path the previous round added (`server size=... expected=...`). Same logic is applied to both `upload_file()` and `upload_bytes()` so the legacy A1-compatibility manual-transfer path is covered identically. **Tests:** the two existing regressions from the previous round (`test_upload_426_data_stream_failure_returns_false`, `test_upload_bytes_426_data_stream_failure_returns_false`) are renamed and split: `test_upload_426_with_intact_file_proceeds` (SIZE matches → returns True, the reporter's case), `test_upload_426_with_truncated_file_returns_false` (SIZE smaller than expected → still fails, the original bug we don't want to regress), `test_upload_426_with_size_check_failing_returns_false` (SIZE itself raises → assume the worst), plus parallel coverage for `upload_bytes()`. The intact-file tests have to inject `SIZE` explicitly because the pyftpdlib mock only flushes the on-disk file after a clean voidresp — which doesn't happen when we monkeypatch voidresp to raise — and the docstring spells that out for future readers. 87 FTP unit tests green; ruff clean. The View-Timelapse-greyed-out behaviour the original #1417 report flagged stays untouched here — once the reporter confirms their upload reliability is back, that diagnosis continues on a healthy install. + - **AMS: physically-empty slots now consistently report state=9 (#1322 follow-up, diagnosed by @RosdasHH)** — Reporter dug into the BambuStudio source and pointed out that our previous fix only caught the narrow `{"id": N}` bare-payload shape, which Bambu firmware only sends right after a printer restart. In steady-state operation — including the more common post-Reset-Slot path on P1S and the A1 Mini BMCU — firmware sends a populated payload with stale fields and signals emptiness via the `tray_exist_bits` bitmask instead. Bambuddy already parsed that bitmask at `bambu_mqtt.py:1758` (`slot_exists = (tray_exist_bits >> global_bit) & 1`) and used it to wipe stale `tray_type` / `tray_color` / `tag_uid` fields, but never promoted the slot's `state`. So downstream readers — the API serializer at `printers.py:457`, the `tray_state in {9, 10}` short-circuit in `inventory.py:1358`, the AMS card — all saw `state: null` and had to guess from absent payload fields. Reporter's API screenshot showed exactly that shape: `state: null, tray_color: null, remain: 0, ...`. Fix lifts a `tray["state"] = 9` assignment to the outer `if not slot_exists` branch (was nested inside the stale-data-clear branch), so the bitmask path now writes the canonical "no spool" state for every empty slot regardless of whether stale fields are present. Hard-typed as `int` 9, not string `"9"` — the downstream check at `inventory.py:1358` uses `tray_state == 9` (not `in {"9", 9}`), so a string would have silently missed and the reporter's deadlock would have come right back. The previous narrow heuristic in `printer_manager.py:797-798` (the `len(tray) == 1 and "id" in tray` shape detector) stays in place as belt-and-suspenders for the post-restart bare-payload edge that bypasses the AMS merge — costs nothing and protects against any MQTT path that doesn't flow through `_handle_ams_data`. The `state: null` surfacing on the API resolves automatically since `printers.py` reads `tray_data.get("state")` directly. **Tests:** two new in `test_bambu_mqtt.py::TestAMSDataHandling` — `test_tray_exist_bits_promotes_empty_slot_to_state_9` exercises the steady-state populated-payload path (slot occupied → bitmask flips bit 1 to 0 → state=9, type-asserted as int; loaded sibling slot keeps its state=11 unchanged); `test_tray_exist_bits_does_not_change_state_on_loaded_slots` pins the negative path (bitmask bit=1 with state=3 leaves state untouched — transitional firmware states like "unloading" don't get corrupted). The two `printer_manager.py` regression tests for the narrow heuristic (`test_bare_tray_emulates_state_9`, `test_populated_payload_with_empty_state_3_is_not_promoted`) stay green — that path is unchanged. 397 mqtt+printer-manager unit tests + 50 inventory/Spoolman slot-assignment integration tests = 447 affected tests green; ruff clean. **UI: visual distinction between physically empty and unconfigured slots (in the same drop).** With the data layer now consistent, the AMS slot card surfaces what Bambuddy actually knows about each empty slot, without overclaiming. New helper `getEmptySlotKind(tray)` in `PrintersPage.tsx` returns `"physical"` (state ∈ {9, 10} — firmware positively confirmed no spool), `"reset"` (any other empty state — could be a user-cleared assignment, mid-unload, or just a slot the firmware hasn't reported on yet), or `null` (loaded). The inline label below the slot circle reads `t('ams.slotEmpty')` ("Empty") uniformly for any empty slot (regular AMS, HT, external) so users get a consistent label everywhere — the previous version only said "Empty" for firmware-confirmed state=9 slots and fell back to an em-dash otherwise, which surfaced as "Empty" for regular AMS slots but "—" for HT AMS (skipped by the bitmask loop) and external trays (separate MQTT path entirely). The state distinction now lives only on the **border** and **hover card** where it doesn't surprise. `FilamentSlotCircle` gains an `emptyKind` prop that picks a quieter dashed border colour for unconfigured slots (`#3d3d3d` vs `#666`), so the visual hierarchy reads "loaded > unconfigured > physically empty" at a glance even though the inline text only differentiates physical from everything else. `EmptySlotHoverCard` gains a `kind` prop and switches the hover label between `ams.emptySlot` ("Empty slot") for physical and the new `ams.emptySlotReset` ("No filament assigned") for everything else — also rewritten from the original "Slot reset — no spool assigned" for the same overclaim reason. All three slot-render call sites in `PrintersPage` (regular AMS grid, HT AMS single-slot, external spool tray) now compute and pass the kind. i18n: 2 new keys (`slotEmpty`, `emptySlotReset`) translated across all 8 locales; parity at 4854 leaves. New test `#1322: empty slot kind is "physical" when state=9 and "reset" otherwise` in `PrintersPage.test.tsx` reuses the existing `phase13EmptySlotProps` mock to capture the `kind` prop across a 4-slot fixture (state=9 / state=3 / state=null / loaded) and asserts each variant flows through. 71 PrintersPage + FilamentHoverCard tests green; build clean. - **Stats page: Filament Used, By Time, and Success Rate now agree with Total Consumed and Total Prints (#1390 follow-up, reported by @IndividualGhost1905)** — After the archived-spool fix shipped the reporter confirmed it worked and gently flagged the round Bambuddy had explicitly postponed: Quick Stats `Filament Used` / `Filament Cost` didn't match `Total Consumed` on the Inventory page; Printer Stats `By Time` didn't match Quick Stats `Print Time`; the success-rate gauge percentage didn't relate to the `Total Prints` count shown right above it. Three independent root causes, fixed together. **(1) Filament Used vs Total Consumed.** `_compute_run_filament_grams` in `main.py` short-circuited to the slicer estimate for `status == "completed"` even when inventory had measured the actual AMS weight delta — the comment on the old test literally said "the print is done, so the full estimate is the right answer." That made Stats and Inventory two different sources of truth: Stats showed slicer-estimate grams, Inventory showed AMS-tracked grams, and the two numbers naturally diverged (slicer estimates are typically a few percent off real consumption). Fixed by reordering the helper so the tracked spool delta (sum of `usage_results[].weight_used` — same source that drives the per-spool `weight_used` counter behind Total Consumed) takes priority for *every* status. The slicer estimate stays as the fallback when no inventory was tracked for the print, and the partial-progress scale stays as the fallback for failed/cancelled/stopped with no tracker — so the existing #1378 partial-aware behaviour is preserved. The `_run_cost` block right next to it already used this priority order, so cost was always tracker-first; only `filament_used_grams` was inconsistent. New prints now record what was actually consumed, so Stats and Inventory show identical numbers. **(2) Printer Stats By Time vs Quick Stats Print Time.** `/archives/slim` only populated `actual_time_seconds` when `status == "completed"`. For failed/cancelled rows the field stayed null and the frontend (`StatsPage.tsx::PrinterStatsWidget`) fell back to `print_time_seconds` — the slicer's *estimated full-print duration*, which is the wrong number for a print that failed at 15% progress. Quick Stats `total_print_time_hours` already counted every event's elapsed `duration_seconds` regardless of status, so the two halves of the page disagreed by the (estimate − actual-elapsed) gap on every non-completed event. Dropped the `status == "completed"` gate in the slim row's `actual_time_seconds` computation; failed/cancelled events now report their measured elapsed time and the frontend's `actual || print_time` fallback chain only ever falls through to the slicer estimate for events with no measured duration at all. **(3) Success Rate %.** Formula was `successful / (successful + failed)`, denominator excluding `cancelled` / `stopped` / any other status. Combined with the visible "Total Prints: N" label right above the gauge, that produced confusing numbers: 4 successful, 0 failed, 48 cancelled showed 100% out of an apparent 52 prints. Switched to `successful / total_prints` — straightforward "what fraction of all attempts succeeded", matches the count the user reads from the widget header. The widget's `stats` prop already exposed `total_prints` so no type changes were needed. **(4) Records widget "Longest Print" — knock-on from (2).** Before (2), `actual_time_seconds` was null for non-completed rows so the Records widget's `findMax(a => a.actual_time_seconds)` implicitly only ranked successful prints. Once (2) populated the field for failed/cancelled events too, an aborted 25-hour print would have outranked a genuinely successful 18-hour print as "Longest Print" — a real semantic regression. Added a `status === 'completed'` gate on the longest getter only, restoring the pre-fix semantic. The other two records (Heaviest Print, Most Expensive) already included non-completed events via `filament_used_grams` and `cost` and intentionally stay as-is, since those values are populated by the partial-progress / tracker logic in `_compute_run_filament_grams` and were never gated on status. **Out of scope** — backfilling the 52 historical events on the reporter's database: `PrintLogEntry.filament_used_grams` is already baked in as the slicer estimate for older prints and we don't store per-event AMS deltas separately to backfill from. The reporter said upfront she'd "reset all statistics and start over" to track new prints cleanly, so this lands without a migration. Similarly the Failure Analysis 30-day default (a separate divergence the agent surfaced while mapping the page) wasn't part of the reporter's complaints and stays untouched. **Tests:** `test_run_filament_helper.py::test_completed_returns_estimate_even_when_tracked_differs` was renamed and inverted to `test_completed_prefers_tracked_over_estimate` — it now pins the new contract (completed + tracker → tracker value), guarding against a future "trust the estimate again" refactor. All 13 existing helper tests still green; the helper's contract changed in exactly one place and the rest (no-tracker fallback, partial-progress scaling, multi-slot summation) is unchanged. `test_archives_api.py::test_slim_actual_time_null_for_failed` was renamed to `test_slim_actual_time_for_failed_includes_elapsed` and inverted — same pattern, the old assertion is now the regression check. `StatsPage.test.tsx` gains two: `uses total_prints as denominator so cancelled/stopped events count (#1390)` (40 successful / 20 failed / 40 cancelled-or-stopped = 40%, matches Total Prints: 100, where the old formula would have shown 67%); and `Longest Print excludes failed prints (#1390)` pinning that an aborted 25-hour run can't outrank a successful 8-hour print as the Longest Print record — protects against a future refactor that removes the new status gate inside `findMax`. 33 StatsPage tests + 66 archive-API + run-filament tests green; frontend build clean. diff --git a/backend/app/services/bambu_ftp.py b/backend/app/services/bambu_ftp.py index 56135ef18..9fb8a3be6 100644 --- a/backend/app/services/bambu_ftp.py +++ b/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 diff --git a/backend/tests/unit/services/test_bambu_ftp.py b/backend/tests/unit/services/test_bambu_ftp.py index 58fa08e5d..6ccb2c1c7 100644 --- a/backend/tests/unit/services/test_bambu_ftp.py +++ b/backend/tests/unit/services/test_bambu_ftp.py @@ -386,16 +386,71 @@ 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. + 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. - 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. + 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() + client.connect() + + 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, "truncated file (SIZE mismatch) must fail" + client.disconnect() + + 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 @@ -407,25 +462,55 @@ class TestUpload: 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, "Upload must fail on 426 to prevent dispatching a truncated file" + assert result is False 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_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 client = ftp_client_factory() client.connect() + data = b"x" * 1024 def raise_426(): raise ftplib.error_temp("426 Failure reading network stream.") - client._ftp.voidresp = raise_426 + def fake_size(_path): + return 1024 # printer-side file matches expected size - result = client.upload_bytes(b"x" * 1024, "/cache/bytes.bin") + 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()