Skip to content

Commit 5eb6ceb

Browse files
committed
fix(transfer): structured envelope on connection errors (BE-2454)
comfy download / comfy upload wrapped their HTTP calls in `except urllib.error.HTTPError` only. HTTPError is a subclass of URLError, so a connection-level failure — connection refused, DNS failure, timeout, TLS error, or a reset mid-transfer — raised the parent URLError, a bare TimeoutError, or a bare ConnectionError and escaped as an unhandled traceback, breaking --json / NDJSON consumers that expect a structured error envelope. Add a second except arm at both call sites (execute_download and execute_upload) catching (URLError, TimeoutError, ConnectionError) and emitting the existing download_failed / upload_failed envelope with the underlying reason in details. HTTPError stays first for its status-specific message; ConnectionError covers a mid-stream reset without swallowing disk-write OSErrors. Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
1 parent c6d5e64 commit 5eb6ceb

3 files changed

Lines changed: 132 additions & 0 deletions

File tree

comfy_cli/command/transfer.py

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -283,6 +283,20 @@ def execute_upload(
283283
details={"status": status, "filename": filename},
284284
)
285285
raise typer.Exit(code=1)
286+
except (urllib.error.URLError, TimeoutError, ConnectionError) as e:
287+
# A connection-level failure (refused / DNS / timeout / TLS at
288+
# connect, or a reset / read-timeout mid-transfer) raises URLError,
289+
# a bare TimeoutError, or a bare ConnectionError — never HTTPError.
290+
# Surface it as a structured envelope instead of letting an
291+
# unhandled traceback break machine/NDJSON consumers.
292+
reason = getattr(e, "reason", None) or e
293+
renderer.error(
294+
code="upload_failed",
295+
message=f"Failed to upload {filename}: {reason}",
296+
hint="check that the server is reachable",
297+
details={"filename": filename, "reason": str(reason)},
298+
)
299+
raise typer.Exit(code=1)
286300

287301
cloud_name = result.get("name", filename)
288302
subfolder = result.get("subfolder", "")
@@ -516,6 +530,20 @@ def execute_download(
516530
details={"status": e.code, "url": url, "index": idx},
517531
)
518532
raise typer.Exit(code=1)
533+
except (urllib.error.URLError, TimeoutError, ConnectionError) as e:
534+
# A connection-level failure (refused / DNS / timeout / TLS at
535+
# connect, or a reset / read-timeout mid-transfer) raises URLError,
536+
# a bare TimeoutError, or a bare ConnectionError — never HTTPError.
537+
# Surface it as a structured envelope instead of letting an
538+
# unhandled traceback break machine/NDJSON consumers.
539+
reason = getattr(e, "reason", None) or e
540+
renderer.error(
541+
code="download_failed",
542+
message=f"Failed to download output {idx}: {reason}",
543+
hint="check that the server is reachable and the output URL is correct",
544+
details={"url": url, "index": idx, "reason": str(reason)},
545+
)
546+
raise typer.Exit(code=1)
519547

520548
file_size = local_path.stat().st_size
521549
entry: dict[str, Any] = {

tests/comfy_cli/command/test_transfer_download.py

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,10 +10,12 @@
1010
from __future__ import annotations
1111

1212
import json
13+
import urllib.error
1314
from pathlib import Path
1415
from unittest.mock import patch
1516

1617
import pytest
18+
import typer
1719

1820
from comfy_cli import comfy_client, jobs_state
1921
from comfy_cli.comfy_client import extract_output_entries
@@ -378,6 +380,74 @@ def test_ndjson_mode_emits_no_human_progress_line(self, fake_target, tmp_path, c
378380
assert "downloaded" not in captured.err
379381

380382

383+
class TestDownloadConnectionError:
384+
"""A connection-level failure — server unreachable, DNS failure, read
385+
timeout, TLS error — raises the parent ``URLError`` (or a bare
386+
``TimeoutError`` on a read timeout), not ``HTTPError``. It must surface as a
387+
structured ``download_failed`` envelope with stdout staying pure JSON, never
388+
an unhandled traceback that breaks ``--json``/NDJSON consumers (BE-2454)."""
389+
390+
def _run(self, fake_target, tmp_path, side_effect) -> typer.Exit:
391+
with (
392+
patch("comfy_cli.command.transfer.resolve_target", return_value=fake_target),
393+
patch.object(transfer._DOWNLOAD_OPENER, "open", side_effect=side_effect),
394+
):
395+
with pytest.raises(typer.Exit) as excinfo:
396+
transfer.execute_download(PROMPT_ID, out_dir=str(tmp_path / "out"))
397+
return excinfo.value
398+
399+
def test_urlerror_emits_download_failed_envelope(self, fake_target, tmp_path, capsys):
400+
_write_state(fake_target)
401+
set_renderer(Renderer(mode=OutputMode.JSON, command="download"))
402+
403+
def _refused(req):
404+
raise urllib.error.URLError(ConnectionRefusedError(111, "Connection refused"))
405+
406+
exc = self._run(fake_target, tmp_path, _refused)
407+
408+
assert exc.exit_code == 1
409+
out_lines = [ln for ln in capsys.readouterr().out.splitlines() if ln.strip()]
410+
env = json.loads(out_lines[-1])
411+
assert env["ok"] is False
412+
assert env["error"]["code"] == "download_failed"
413+
assert "Connection refused" in env["error"]["message"]
414+
assert "Connection refused" in env["error"]["details"]["reason"]
415+
416+
def test_bare_timeout_is_also_caught(self, fake_target, tmp_path, capsys):
417+
_write_state(fake_target)
418+
set_renderer(Renderer(mode=OutputMode.JSON, command="download"))
419+
420+
def _timeout(req):
421+
raise TimeoutError("timed out")
422+
423+
exc = self._run(fake_target, tmp_path, _timeout)
424+
425+
assert exc.exit_code == 1
426+
env = json.loads([ln for ln in capsys.readouterr().out.splitlines() if ln.strip()][-1])
427+
assert env["error"]["code"] == "download_failed"
428+
429+
def test_connection_reset_mid_stream_is_caught(self, fake_target, tmp_path, capsys):
430+
_write_state(fake_target)
431+
set_renderer(Renderer(mode=OutputMode.JSON, command="download"))
432+
433+
class _ResetResp:
434+
def read(self, n):
435+
raise ConnectionResetError(104, "Connection reset by peer")
436+
437+
def __enter__(self):
438+
return self
439+
440+
def __exit__(self, *exc):
441+
return False
442+
443+
exc = self._run(fake_target, tmp_path, lambda req: _ResetResp())
444+
445+
assert exc.exit_code == 1
446+
env = json.loads([ln for ln in capsys.readouterr().out.splitlines() if ln.strip()][-1])
447+
assert env["error"]["code"] == "download_failed"
448+
assert "reset by peer" in env["error"]["message"]
449+
450+
381451
# ---------------------------------------------------------------------------
382452
# _default_out_dir — project/1 root wins over the legacy config key
383453
# ---------------------------------------------------------------------------

tests/comfy_cli/command/test_transfer_upload.py

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
from pathlib import Path
1515

1616
import pytest
17+
import typer
1718

1819
from comfy_cli.command import transfer
1920
from comfy_cli.target import Target
@@ -145,3 +146,36 @@ def test_json_mode_stdout_is_pure_json_no_human_line(self, asset, monkeypatch, c
145146
assert parsed[-1]["data"]["uploads"][0]["cloud_name"] == "ab12.png"
146147
assert "uploaded" not in captured.out
147148
assert "uploaded" not in captured.err
149+
150+
151+
class TestUploadConnectionError:
152+
"""A connection-level failure (``URLError``/``TimeoutError``) on upload must
153+
surface as a structured ``upload_failed`` envelope, not an unhandled
154+
traceback that breaks ``--json``/NDJSON consumers (BE-2454)."""
155+
156+
@pytest.fixture(autouse=True)
157+
def reset_renderer(self):
158+
from comfy_cli.output.renderer import reset_renderer_for_testing
159+
160+
reset_renderer_for_testing()
161+
yield
162+
reset_renderer_for_testing()
163+
164+
def test_urlerror_emits_upload_failed_envelope(self, asset, monkeypatch, capsys):
165+
from comfy_cli.output import Renderer, set_renderer
166+
from comfy_cli.output.renderer import OutputMode
167+
168+
err = urllib.error.URLError(ConnectionRefusedError(111, "Connection refused"))
169+
monkeypatch.setattr(transfer, "_TRANSFER_OPENER", _FakeOpener(error=err))
170+
monkeypatch.setattr(transfer, "resolve_target", lambda where=None: _local_target())
171+
set_renderer(Renderer(mode=OutputMode.JSON, command="upload"))
172+
173+
with pytest.raises(typer.Exit) as excinfo:
174+
transfer.execute_upload([str(asset)], where="local")
175+
176+
assert excinfo.value.exit_code == 1
177+
env = json.loads([ln for ln in capsys.readouterr().out.splitlines() if ln.strip()][-1])
178+
assert env["ok"] is False
179+
assert env["error"]["code"] == "upload_failed"
180+
assert "Connection refused" in env["error"]["message"]
181+
assert "Connection refused" in env["error"]["details"]["reason"]

0 commit comments

Comments
 (0)