Skip to content

curl: stop retrying local download write failures - #24275

Merged
MikeMcQuaid merged 1 commit into
Homebrew:mainfrom
BrandenXia:curl-local-write-errors
Oct 17, 2026
Merged

MikeMcQuaid merged 1 commit into
Homebrew:mainfrom
BrandenXia:curl-local-write-errors

Conversation

@BrandenXia

@BrandenXia BrandenXia commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

When a download runs out of local disk space, curl exits with CURLE_WRITE_ERROR (23). Homebrew currently wraps this as an ordinary download failure, tries any mirrors and lets RetryableDownload repeat the transfer. Changing servers cannot fix a local write failure, and the existing error does not identify the destination or suggest checking storage.

Raise a separate DownloadWriteError from Utils::Curl.curl_download for exit 23. This bypasses mirror fallback and download retries, reports the destination with guidance to check free space and permissions, and retains curl's diagnostic and the original exception as the cause. Partial downloads remain available for a later resume. Other curl failures keep their existing handling. Exit 23 also covers permission and other local write errors, so the message deliberately does not assert that the disk is full. See curl's error-code documentation.

This change covers curl download write failures; it does not estimate space required to unpack or build packages.

Reproduction

On a machine whose Homebrew cache volume is full, run brew fetch --retry llvm. Before this change, a local write failure is handled as a retryable download error. After this change, the affected download stops with its destination and actionable guidance.

For a bounded reproduction without filling a disk, run this from the repository root. A temporary file-size limit causes the same curl exit 23:

./bin/brew ruby -- - <<'RUBY'
require "utils/curl"
require "tmpdir"

Dir.mktmpdir do |directory|
  source = File.join(directory, "source")
  File.binwrite(source, "x" * 16_384)
  Signal.trap("XFSZ", "IGNORE")
  Process.setrlimit(Process::RLIMIT_FSIZE, 1024)
  Utils::Curl.curl_download("file://#{source}",
                            to: File.join(directory, "download.incomplete"), retries: 0)
end
RUBY

Validation

  • Regression tests failed before the fix and passed afterwards using the exact example targets: ./bin/brew tests --only=utils/curl:869,utils/curl:887,download_strategies/curl:116,retryable_download:15.
  • ./bin/brew lgtm --online passed: global type checking, style and the relevant tests.
  • A real local curl transfer under a 1 KiB file-size limit produced exit 23, raised DownloadWriteError, preserved the original exception and retained 1,024 partial bytes.
  • Tests also cover unchanged propagation of network failures and preservation of partial downloads.

  • Have you followed our Contributing guidelines?
  • Have you checked for other open Pull Requests for the same change?
  • Have you explained what your changes do? Performance claims (e.g. "this is faster") must include brew benchmark results.
  • Have you explained why you'd like these changes included, not just what they do?
  • For bug fixes, have you given step-by-step brew commands to reproduce the bug?
  • Have you written new tests (excluding integration tests)? Here's an example.
  • Have you successfully run brew lgtm (style, typechecking and tests) locally?

  • I did not use AI/LLM to create this PR, or I disclosed the tool/model below and reviewed its output; I did not attribute commits to AI and will answer maintainer questions and review comments myself without AI/LLM.

OpenAI Codex (GPT-6) investigated the failure path, wrote the change and regression tests, and ran the validation above. I reviewed the code and made sure that this is not slopping.


Summary by CodeRabbit

  • Bug Fixes
    • Local file write failures during downloads now raise a specific error with the destination path and guidance on checking disk space and write permissions.
    • Downloads are no longer retried from mirrors when the failure is caused by writing to the local filesystem. Other download errors continue to be reported unchanged.

@BrandenXia

Copy link
Copy Markdown
Contributor Author

btw, when extracting the download contents and there's insufficient storage, the failure also isn't handled and create a bunch of unreadable error messages. i might open a pr later to fix that after this one is done

@BrandenXia
BrandenXia marked this pull request as ready for review October 17, 2026 00:53
@coderabbitai

coderabbitai Bot commented Oct 17, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 24624e74-9882-4e55-8172-43afded5be19
📥 Commits

Reviewing files that changed from the base of the PR and between 19fe946 and 5a91cbf.

📒 Files selected for processing: 5
  • Library/Homebrew/exceptions.rb
  • Library/Homebrew/test/download_strategies/curl_spec.rb
  • Library/Homebrew/test/retryable_download_spec.rb
  • Library/Homebrew/test/utils/curl_spec.rb
  • Library/Homebrew/utils/curl.rb

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📝 Walkthrough

curl_download now translates curl exit status 23 into DownloadWriteError. The error includes the destination and write guidance. Tests cover diagnostic details and verify that download retries and mirror fallback do not occur for this failure.

Changes: curl write failure handling

Layer / File(s) Summary
Translate curl write errors
Library/Homebrew/exceptions.rb, Library/Homebrew/utils/curl.rb, Library/Homebrew/test/utils/curl_spec.rb
Adds DownloadWriteError and raises it when curl exits with status 23. The error includes the destination, write guidance, and cleaned stderr. Tests cover the cause, partial data, and unchanged propagation of other errors.
Verify download retry behaviour
Library/Homebrew/test/download_strategies/curl_spec.rb, Library/Homebrew/test/retryable_download_spec.rb
Tests verify that status 23 raises DownloadWriteError after one curl call, even when mirrors or multiple tries are configured.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 5a91c

The change is mergeable after normal checks; no unresolved issue is established.

Security architecture risk: 🔵 Low · up to 5a91c

The change stops futile retries without treating failed downloads as complete. Some callers will now stop instead of using cached data, creating a limited compatibility risk. No introduced security-control bypass was established.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The demonstrated exposure is failure handling in the invoking Homebrew process and its download/cache operations. The new exception does not itself add a network entrypoint or grant additional filesystem authority.

Trust Boundaries and Controls

  • observed — The new diagnostic reads stderr from ErrorDuringExecution after that exception has redacted configured secrets from its stored output. Cleaning progress text therefore does not bypass the existing configured-secret redaction control.

Resilience and Maintainability Implications

  • observed — For vulnerability feeds, exit 23 now prevents stale-cache fallback. The refresh still validates a separate temporary file before replacing the cache, so the newly propagated exception occurs before publication rather than accepting a truncated feed.
Pre-merge checks: Passed 4
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: preventing retries for local download write failures.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@MikeMcQuaid MikeMcQuaid left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, thanks!

@MikeMcQuaid
MikeMcQuaid added this pull request to the merge queue Oct 17, 2026
Merged via the queue into Homebrew:main with commit b5ba282 Oct 17, 2026
55 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

⌂ 换网址