fix: retry transient HTTP statuses and show friendly download errors#432
Merged
fix: retry transient HTTP statuses and show friendly download errors#432
Conversation
- HTTP 408/429/500/502/503/504 are now retried (up to 3 attempts with backoff) rather than failing immediately. Transient server-side errors can now self-heal instead of aborting the download. - DownloadException raised from comfy model download is rendered as a one-line red error with exit code 1, instead of a full Rich-panel Python traceback. comfy node install uses the existing display_error_message path. - Download progress bars now use transient=True so they disappear when the download completes. Running many downloads in sequence no longer leaves a stack of persistent bars. Signed-off-by: Alexander Piskun <[email protected]>
📝 WalkthroughWalkthroughThis pull request enhances download resilience and error handling across the CLI. It introduces transient HTTP status detection with automatic retries, improves error messages, ensures clean CLI exits on failures, and prevents console visual glitches through transient progress displays. Comprehensive tests validate the new error-handling paths. Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #432 +/- ##
==========================================
+ Coverage 76.63% 77.15% +0.52%
==========================================
Files 35 35
Lines 4279 4311 +32
==========================================
+ Hits 3279 3326 +47
+ Misses 1000 985 -15
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
Non-retriable httpx errors (UnsupportedProtocol, TooManyRedirects, DecodingError, etc.) previously escaped download_file and surfaced as a full Typer-rendered traceback. They are now caught and converted to DownloadException so the existing catches in model download and registry-install render them as a one-line friendly error. Also adds tests for the new wrapping and for the registry-install DownloadException catch path. Signed-off-by: Alexander Piskun <[email protected]>
…ndering DownloadException messages can contain server-controlled text (e.g. a 401 body's "message" field, echoed back via guess_status_code_reason). If that text contained rich-markup metacharacters it would either silently strip bracketed substrings (e.g. "[id]" in a URL) or, for a stray "[/]" or unbalanced closing tag, raise rich.errors.MarkupError — which escaped both catch sites and surfaced as the exact Typer traceback panel #430 set out to suppress. - ui.display_error_message: use markup=False and the style= kwarg so the message is never parsed as markup. - models.download exception rendering: escape the DownloadException message before embedding it in the [bold red]...[/bold red] template. New tests cover: plain messages, unbalanced / closing / style-like bracketed tokens, and an end-to-end CLI invocation where download_file raises a DownloadException carrying markup characters. Signed-off-by: Alexander Piskun <[email protected]>
…_file
- _friendly_network_error now includes the stdlib http.HTTPStatus phrase
alongside the code ("HTTP 500 Internal Server Error" instead of just
"HTTP 500"), so the retry-exhaust DownloadException carries what the
status means — not only its number. Unknown codes fall back cleanly.
- httpx.InvalidURL inherits directly from Exception, not httpx.HTTPError,
so the previous except clause would have let it leak as a raw Typer
traceback. Add it to the non-retriable catch tuple and to the friendly
message branches, so a malformed URL surfaces as a one-line error.
Tests: phrase assertions (known + unknown code + unrelated-reason
preservation), InvalidURL wrap behaviour (single call, no retry,
__cause__ chain, pre-existing file preserved), and retry-exhaust
message now asserts the phrase.
Signed-off-by: Alexander Piskun <[email protected]>
Before this change, a failed custom-node download in registry-install returned normally after displaying the error, giving exit code 0. Automation that chains `comfy node registry-install X && ...` would treat the failure as success. Raise typer.Exit(code=1) so the failure is visible to CI / shell pipelines, matching the pattern used in models.py download. Update the regression tests to assert the non-zero exit code. Signed-off-by: Alexander Piskun <[email protected]>
dosubot
Bot
added
size:M
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
comfy_cli/command/custom_nodes/command.py (1)
1144-1152:⚠️ Potential issue | 🟠 MajorReturn a non-zero exit for pre-download registry failures.
These paths print “Failed to download” but then
return, soregistry-installexits successfully even when the registry omitted a URL or the install API failed. That lets the exit-code goblin sneak past CI.🐛 Proposed fix
if not node_version.download_url: logging.error("Download URL not provided from the registry.") ui.display_error_message(f"Failed to download the custom node {node_id}.") - return + raise typer.Exit(code=1) except Exception as e: logging.error(f"Encountered an error while installing the node. error: {str(e)}") ui.display_error_message(f"Failed to download the custom node {node_id}.") - return + raise typer.Exit(code=1) from None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@comfy_cli/command/custom_nodes/command.py` around lines 1144 - 1152, The code paths that detect missing node_version.download_url or catch Exception only log and return, causing the CLI command (registry-install) to exit with a zero status; change those early returns to exit with a non-zero code (e.g., call sys.exit(1) or raise SystemExit(1)) so failures propagate to the process exit status. Update the branches that log via logging.error(...) / ui.display_error_message(...) (the block checking node_version.download_url and the except Exception as e handler) to call sys.exit(1) after logging instead of plain return; ensure sys is imported if needed and keep the same error messages and node_id variable in the display text.comfy_cli/command/models/models.py (1)
315-316:⚠️ Potential issue | 🟡 MinorRoute empty filename errors through the friendly CLI path.
This
DownloadExceptionis raised before the new handler, socomfy model download --filename "" ...can still show the raw exception/traceback instead of a clean red error.🐛 Proposed fix
if local_filename is None: raise typer.Exit(code=1) if local_filename == "": - raise DownloadException("Filename cannot be empty") + ui.display_error_message("Filename cannot be empty") + raise typer.Exit(code=1) from None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@comfy_cli/command/models/models.py` around lines 315 - 316, The empty-filename check in models.py currently raises DownloadException (if local_filename == "") which bypasses the new friendly CLI handler; change this to raise the CLI-friendly error type used by the new handler (e.g., FriendlyCLIError or the project's CLIError/ClickException wrapper) with the same message so the new handler will format it nicely (replace the DownloadException raise at the local_filename check with a raise of the friendly CLI error class).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@comfy_cli/command/custom_nodes/command.py`:
- Around line 1144-1152: The code paths that detect missing
node_version.download_url or catch Exception only log and return, causing the
CLI command (registry-install) to exit with a zero status; change those early
returns to exit with a non-zero code (e.g., call sys.exit(1) or raise
SystemExit(1)) so failures propagate to the process exit status. Update the
branches that log via logging.error(...) / ui.display_error_message(...) (the
block checking node_version.download_url and the except Exception as e handler)
to call sys.exit(1) after logging instead of plain return; ensure sys is
imported if needed and keep the same error messages and node_id variable in the
display text.
In `@comfy_cli/command/models/models.py`:
- Around line 315-316: The empty-filename check in models.py currently raises
DownloadException (if local_filename == "") which bypasses the new friendly CLI
handler; change this to raise the CLI-friendly error type used by the new
handler (e.g., FriendlyCLIError or the project's CLIError/ClickException
wrapper) with the same message so the new handler will format it nicely (replace
the DownloadException raise at the local_filename check with a raise of the
friendly CLI error class).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 60bf0838-6106-489d-84f3-b542f8a6dd08
📒 Files selected for processing (8)
comfy_cli/command/custom_nodes/command.pycomfy_cli/command/models/models.pycomfy_cli/file_utils.pycomfy_cli/ui.pytests/comfy_cli/command/models/test_models.pytests/comfy_cli/command/nodes/test_node_install.pytests/comfy_cli/test_ui.pytests/test_file_utils_network.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #430.
Three focused changes that together address the bug report (visual glitches + verbose tracebacks when pasting many
comfy model downloadcommands into a shell):_download_file_httpxnow raises a private_TransientHTTPStatusErrorfor statuses in{408, 429, 500, 502, 503, 504}, which the existing retry loop catches alongside network-level errors. CivitAI CDN blips and other transient server errors can now self-heal instead of aborting the download on the first attempt.comfy model downloadcatchesDownloadExceptionand prints a one-line red error before exiting with code 1, replacing the previous ~100-line Rich-panel Python traceback.comfy node installcatches it the same way, using the existingui.display_error_messagepath.ui.show_progressand_poll_aria2_downloadnow usetransient=True, so each progress bar is erased when its download completes. Running manycomfy model downloadcommands in sequence no longer leaves 30 persistent bars stacked in the terminal.Notes / follow-ups
Retry-Afterheader on 429 responses is not honored; fixed 2 s / 4 s backoff is used. Worth a follow-up if we see CivitAI or similar providers sending it.huggingface_hub.hf_hub_download) are not touched — they have their own error semantics.raise DownloadException("Filename cannot be empty")atcomfy_cli/command/models/models.py:315still produces a traceback because it sits above the newtry/except; unrelated to [Potential bug] Visual glitches and error #430 and can be cleaned up separately.