fix(transfer): structured envelope on connection errors (BE-2454)#492
fix(transfer): structured envelope on connection errors (BE-2454)#492bigcat88 wants to merge 3 commits into
Conversation
dosubot
Bot
added
the
size:S
📝 WalkthroughWalkthrough
ChangesTransfer connection error handling
Sequence Diagram(s)sequenceDiagram
participant Test as TestUploadConnectionError
participant Transfer as execute_upload
participant Opener as _TRANSFER_OPENER
Test->>Transfer: call execute_upload
Transfer->>Opener: open file upload
Opener-->>Transfer: raise URLError / IncompleteRead
Transfer->>Transfer: render upload_failed envelope
Transfer-->>Test: raise typer.Exit(code=1)
sequenceDiagram
participant Test as TestDownloadConnectionError
participant Transfer as execute_download
participant Opener as _DOWNLOAD_OPENER
Test->>Transfer: call execute_download
Transfer->>Opener: open target URL
Opener-->>Transfer: raise URLError / TimeoutError / ConnectionResetError / IncompleteRead
Transfer->>Transfer: render download_failed envelope
Transfer->>Transfer: delete partial local_path if present
Transfer-->>Test: raise typer.Exit(code=1)
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
comfy_cli/command/transfer.py (1)
513-546: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDelete partial downloads on failure
resp.read()can fail afterlocal_pathhas already been opened, leaving a half-written file behind. Unlink it in the failure paths (including the size-limit guard), or write to a temp file and rename on success, so a busted download doesn’t masquerade as the real thing — a little gremlin souvenir otherwise.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@comfy_cli/command/transfer.py` around lines 513 - 546, The download flow in transfer handling can leave a partial file behind if resp.read() fails or the max_download guard raises after local_path has been opened. Update the download path in the transfer logic to clean up local_path on every failure path, including the HTTPError, URLError/TimeoutError/ConnectionError handlers and the size-limit check, or switch the download to a temporary file that is only renamed after success. Use the existing download block and related renderer.error handling to locate the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@comfy_cli/command/transfer.py`:
- Around line 286-299: The upload and download paths duplicate the same
connection-error rendering logic, so factor the shared behavior into a small
helper near the existing transfer flow (for example, a helper used by the upload
and download exception blocks). Move the common reason extraction,
renderer.error call, and typer.Exit handling into that helper, and let both call
sites pass their specific code, target label (filename or URL), hint, and extra
details so the two except blocks stay consistent and easier to maintain.
---
Outside diff comments:
In `@comfy_cli/command/transfer.py`:
- Around line 513-546: The download flow in transfer handling can leave a
partial file behind if resp.read() fails or the max_download guard raises after
local_path has been opened. Update the download path in the transfer logic to
clean up local_path on every failure path, including the HTTPError,
URLError/TimeoutError/ConnectionError handlers and the size-limit check, or
switch the download to a temporary file that is only renamed after success. Use
the existing download block and related renderer.error handling to locate the
fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8714f75b-39e3-43a2-81be-e259a2c21bfc
📒 Files selected for processing (3)
comfy_cli/command/transfer.pytests/comfy_cli/command/test_transfer_download.pytests/comfy_cli/command/test_transfer_upload.py
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>
bigcat88
force-pushed
the
bigcat88/be-2454-transfer-urlerror-envelope
branch
from
5eb6ceb to
502394d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@comfy_cli/command/transfer.py`:
- Around line 626-660: The download flow in the transfer logic leaves a partial
file behind whenever an exception occurs after opening local_path for writing,
which can break retries because _collision_safe_path will treat the truncated
file as valid. Update the download path in the relevant open(req) /
open(local_path, "wb") block to either write to a temporary file and rename on
success, or explicitly unlink local_path in the HTTPError and connection-failure
handlers as well as the max_download guard path. Keep the fix centered around
the download routine in transfer.py so all failure cases clean up consistently.
In `@tests/comfy_cli/command/test_transfer_upload.py`:
- Around line 151-181: Upload connection-error coverage is incomplete in
TestUploadConnectionError compared with the download tests. Add sibling cases
alongside test_urlerror_emits_upload_failed_envelope that force
_TRANSFER_OPENER.open to raise a bare TimeoutError and a ConnectionError, then
assert execute_upload still exits with typer.Exit code 1 and emits the same
structured upload_failed JSON envelope. Keep the new tests aligned with
TestDownloadConnectionError and use the existing transfer.execute_upload,
_FakeOpener, and renderer setup so the shared exception handling and fallback
message path are exercised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 52b1f5b9-f8a1-4d64-85b5-ceb2294235ba
📒 Files selected for processing (3)
comfy_cli/command/transfer.pytests/comfy_cli/command/test_transfer_download.pytests/comfy_cli/command/test_transfer_upload.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@comfy_cli/command/transfer.py`:
- Around line 626-660: The download flow in the transfer logic leaves a partial
file behind whenever an exception occurs after opening local_path for writing,
which can break retries because _collision_safe_path will treat the truncated
file as valid. Update the download path in the relevant open(req) /
open(local_path, "wb") block to either write to a temporary file and rename on
success, or explicitly unlink local_path in the HTTPError and connection-failure
handlers as well as the max_download guard path. Keep the fix centered around
the download routine in transfer.py so all failure cases clean up consistently.
In `@tests/comfy_cli/command/test_transfer_upload.py`:
- Around line 151-181: Upload connection-error coverage is incomplete in
TestUploadConnectionError compared with the download tests. Add sibling cases
alongside test_urlerror_emits_upload_failed_envelope that force
_TRANSFER_OPENER.open to raise a bare TimeoutError and a ConnectionError, then
assert execute_upload still exits with typer.Exit code 1 and emits the same
structured upload_failed JSON envelope. Keep the new tests aligned with
TestDownloadConnectionError and use the existing transfer.execute_upload,
_FakeOpener, and renderer setup so the shared exception handling and fallback
message path are exercised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 52b1f5b9-f8a1-4d64-85b5-ceb2294235ba
📒 Files selected for processing (3)
comfy_cli/command/transfer.pytests/comfy_cli/command/test_transfer_download.pytests/comfy_cli/command/test_transfer_upload.py
🛑 Comments failed to post (2)
comfy_cli/command/transfer.py (1)
626-660: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Map the file and inspect the relevant section plus nearby helpers. ast-grep outline comfy_cli/command/transfer.py --view expanded || true echo '--- relevant lines ---' sed -n '560,720p' comfy_cli/command/transfer.py echo '--- search for cleanup/collision helpers ---' rg -n "collision_safe_path|unlink\\(|saved_files|download_failed|max_download|HTTPError|URLError|TimeoutError|ConnectionError" comfy_cli/command/transfer.pyRepository: Comfy-Org/comfy-cli
Length of output: 9632
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect surrounding logic for saved_files accumulation and upload/download flow. rg -n "saved_files|download|collision_safe_path|_DOWNLOAD_OPENER|dest" comfy_cli/command/transfer.py echo '--- file excerpt 1 ---' sed -n '1,220p' comfy_cli/command/transfer.py echo '--- file excerpt 2 ---' sed -n '220,420p' comfy_cli/command/transfer.pyRepository: Comfy-Org/comfy-cli
Length of output: 21446
🏁 Script executed:
#!/bin/bash set -euo pipefail # Determine whether cleanup is already performed elsewhere for the same failure path. rg -n "missing_ok=True|unlink\\(|TemporaryDirectory|NamedTemporaryFile|save.*download|download.*cleanup|finally:" comfy_cli -g '!**/__pycache__/**' echo '--- repo files mentioning collision_safe_path or similar ---' rg -n "collision_safe_path" comfy_cli -g '!**/__pycache__/**'Repository: Comfy-Org/comfy-cli
Length of output: 3137
🏁 Script executed:
#!/bin/bash set -euo pipefail # Use a read-only verifier to see whether a failed transfer leaves a partial file behind # and whether retry logic would treat that path as a collision. python3 - <<'PY' from pathlib import Path import tempfile import os tmp = Path(tempfile.mkdtemp()) p = tmp / "file.bin" # Simulate the stream-writing pattern. with open(p, "wb") as fp: fp.write(b"abc") fp.flush() os.fsync(fp.fileno()) print("exists_after_partial_write", p.exists(), "size", p.stat().st_size) # Simulate retry-style collision handling. def collision_safe_path(path: Path) -> Path: if not path.exists(): return path stem = path.stem suffix = path.suffix i = 1 while True: candidate = path.with_name(f"{stem}_{i}{suffix}") if not candidate.exists(): return candidate i += 1 print("collision_safe_path", collision_safe_path(p)) PYRepository: Comfy-Org/comfy-cli
Length of output: 244
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the transfer flow around where saved_files and destination paths are handled. grep -n "saved_files\|_collision_safe_path\|local_path\|dest" -n comfy_cli/command/transfer.py | head -n 80 echo '--- around path selection and download loop ---' sed -n '480,720p' comfy_cli/command/transfer.pyRepository: Comfy-Org/comfy-cli
Length of output: 11813
Remove partial downloads on failure
Any exception afteropen(local_path, "wb")— including mid-transfer resets/timeouts and themax_downloadguard — leaves a truncated file behind indest, and retries will dodge it via_collision_safe_path. Unlink the path on error or switch to a temp-file-then-rename flow so the download doesn’t leave a goblin in the output dir.🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 629-629: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(local_path, "wb")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').(open-filename-from-request)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@comfy_cli/command/transfer.py` around lines 626 - 660, The download flow in the transfer logic leaves a partial file behind whenever an exception occurs after opening local_path for writing, which can break retries because _collision_safe_path will treat the truncated file as valid. Update the download path in the relevant open(req) / open(local_path, "wb") block to either write to a temporary file and rename on success, or explicitly unlink local_path in the HTTPError and connection-failure handlers as well as the max_download guard path. Keep the fix centered around the download routine in transfer.py so all failure cases clean up consistently.tests/comfy_cli/command/test_transfer_upload.py (1)
151-181: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Upload connection-error coverage is thinner than download's — only URLError, no TimeoutError or ConnectionError.
The download test class exercises all three exception types the shared
exceptclause catches (URLError, bareTimeoutError,ConnectionResetError), but upload only coversURLError. Sinceexecute_uploadcatches the identical tuple, add sibling tests for a bareTimeoutErrorand aConnectionErrorfrom_TRANSFER_OPENER.opento keep parity and actually exercise every branch of theor efallback (which only kicks in for exceptions lacking a.reasonattribute).TestDownloadConnectionError
coversURLError, bareTimeoutError, and mid-streamConnectionResetError, verifying exit code 1 and a JSON-only*_failed` envelope.🧰 Tools
🪛 Pylint (4.0.6)
[convention] 157-157: Missing function or method docstring
(C0116)
[error] 158-158: Unable to import 'comfy_cli.output.renderer'
(E0401)
[convention] 158-158: Import outside toplevel (comfy_cli.output.renderer.reset_renderer_for_testing)
(C0415)
[convention] 164-164: Missing function or method docstring
(C0116)
[warning] 164-164: Redefining name 'asset' from outer scope (line 55)
(W0621)
[error] 165-165: Unable to import 'comfy_cli.output'
(E0401)
[convention] 165-165: Import outside toplevel (comfy_cli.output.Renderer, comfy_cli.output.set_renderer)
(C0415)
[error] 166-166: Unable to import 'comfy_cli.output.renderer'
(E0401)
[convention] 166-166: Import outside toplevel (comfy_cli.output.renderer.OutputMode)
(C0415)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/comfy_cli/command/test_transfer_upload.py` around lines 151 - 181, Upload connection-error coverage is incomplete in TestUploadConnectionError compared with the download tests. Add sibling cases alongside test_urlerror_emits_upload_failed_envelope that force _TRANSFER_OPENER.open to raise a bare TimeoutError and a ConnectionError, then assert execute_upload still exits with typer.Exit code 1 and emits the same structured upload_failed JSON envelope. Keep the new tests aligned with TestDownloadConnectionError and use the existing transfer.execute_upload, _FakeOpener, and renderer setup so the shared exception handling and fallback message path are exercised.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @bigcat88.
Found 5 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 3 |
| 🟢 Low | 2 |
Panel: 6/8 reviewers contributed findings.
Reviewers that did not contribute: kimi-k2.5:adversarial (empty), kimi-k2.5:edge-case (empty)
…-2454) A connection reset / read-timeout during resp.read() can occur after open(local_path, "wb") has created and partially written the destination file. The connection-error handler emitted the download_failed envelope and exited without removing it, leaving a truncated artifact in the out-dir — and since _collision_safe_path never reuses a name, one more per retry, which a downstream consumer scanning the dir could mistake for a valid output. Unlink local_path (missing_ok, so the connect-time case where no file was created is a no-op; OSError-guarded so cleanup can't re-raise on the error path) before raising. Regression test asserts the out-dir is empty after a mid-stream reset. Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
… (BE-2454) resp.read() raises http.client.IncompleteRead when a chunked response body is truncated mid-transfer (the server closes before the final chunk arrives). IncompleteRead is an HTTPException, not a URLError or ConnectionError, so it escaped the connection-error handlers as an unhandled traceback — the same machine/NDJSON-consumer breakage this handling exists to prevent. Add http.client.HTTPException to the except tuple in both execute_upload and execute_download, so a truncated transfer surfaces as the structured upload_failed / download_failed envelope (and, on download, still runs the partial-file cleanup). Tests cover IncompleteRead through both paths. Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
dosubot
Bot
added
size:M
There was a problem hiding this comment.
♻️ Duplicate comments (1)
comfy_cli/command/transfer.py (1)
668-673: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winURL in error details may leak query-string credentials.
details={"url": url, ...}still emits the raw output URL verbatim; ifoutput_urlscarries presigned URLs with auth tokens in the query string, this leaks secrets into the JSON envelope/logs. Not introduced by this diff, but still present — flagging again since it wasn't addressed in this round.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@comfy_cli/command/transfer.py` around lines 668 - 673, The download_failed error payload in transfer.py is still exposing the raw output URL in renderer.error details, which can leak query-string credentials. Update the error handling around the output download path so the details for the failing output use a sanitized URL value instead of the original url, and keep the unique identifiers renderer.error, output_urls, and the details dictionary aligned with the existing download_failed message structure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@comfy_cli/command/transfer.py`:
- Around line 668-673: The download_failed error payload in transfer.py is still
exposing the raw output URL in renderer.error details, which can leak
query-string credentials. Update the error handling around the output download
path so the details for the failing output use a sanitized URL value instead of
the original url, and keep the unique identifiers renderer.error, output_urls,
and the details dictionary aligned with the existing download_failed message
structure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 62d7048a-fa04-4262-9d3c-1a75a0b9527b
📒 Files selected for processing (3)
comfy_cli/command/transfer.pytests/comfy_cli/command/test_transfer_download.pytests/comfy_cli/command/test_transfer_upload.py
|
@bigcat88 Can you check the CI failures on this? ^ |
TLS handshake failure to PyTorch's download mirror - infra/network, server-side. Not related to the changes from this PR |
|
3 participants
comfy downloadandcomfy uploadnow emit a structureddownload_failed/upload_failedenvelope on a connection-level failure (connection refused, DNS failure, timeout, TLS error, or a mid-transfer reset) instead of raising an unhandledurllib.error.URLErrortraceback that broke--json/NDJSON consumers.except (urllib.error.URLError, TimeoutError, ConnectionError)arm after the existingHTTPErrorarm at both call sites incomfy_cli/command/transfer.py(execute_download,execute_upload); the underlying failure reason is surfaced inerror.details.reason.HTTPErroris still caught first for its HTTP-status message;ConnectionErrorcatches a mid-streamresp.read()reset without swallowing disk-writeOSErrors.test_transfer_download.py::TestDownloadConnectionError(URLError, bare TimeoutError, mid-stream ConnectionResetError) andtest_transfer_upload.py::TestUploadConnectionError(URLError), asserting exit 1 and a pure-JSON*_failedenvelope.