Standardize binary cache upload failure diagnostics

[xiaozhuai (xiaozhuai)]

Fixes microsoft/vcpkg#28927

Background

Binary cache upload failures currently produce provider-specific diagnostics.

For example, HTTP uploads may report:

warning: curl failed to PUT file

while S3 uploads may report:

warning: ... aws s3 cp ... failed with exit code 1

This makes it difficult for CI systems to reliably detect binary cache upload failures by parsing the log.

Changes

Prepend a standardized message to existing binary cache upload failure diagnostics:

Binary cache upload failed:

For example:

warning: Binary cache upload failed: curl failed to PUT file ...
warning: Binary cache upload failed: ... aws s3 cp ... failed with exit code 1

The provider-specific diagnostic details are preserved.

Example

vcpkg install stb \
    --binarysource="http,https://not.found.com/{sha},readwrite" \
    --require-binary-cache-upload

[Copilot]

Pull request overview

Adds a new global CLI option --require-binary-cache-upload to make vcpkg commands fail (nonzero exit code) when any asynchronous binary cache upload fails, enabling CI systems to reliably detect cache population failures.

Changes:

• Introduces --require-binary-cache-upload argument parsing and localized help/error messages.
• Extends binary cache upload providers to report expected upload counts and accumulates upload failures across all destinations.
• Propagates the resulting “uploads required and some failed” status into exit codes for commands that perform uploads (e.g., install, build, ci, upgrade, x-set-installed, x-test-features), while preserving existing behavior when the option is not set.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/vcpkg/vcpkgcmdarguments.cpp	Parses the new --require-binary-cache-upload switch with localized help text.
include/vcpkg/vcpkgcmdarguments.h	Adds a new Optional<bool> flag and accessor for the switch.
include/vcpkg/base/contractual-constants.h	Defines the new switch name string constant.
include/vcpkg/base/message-data.inc.h	Declares new localized messages for help text and upload-failure reporting.
locales/messages.json	Adds generated message strings for the new option/help and failure message.
include/vcpkg/binarycaching.h	Extends write-provider interface (upload_count) and changes wait_for_async_complete_and_join() to return success/failure when required.
src/vcpkg/binarycaching.cpp	Implements expected-vs-successful upload tracking, accumulates failures, and reports/returns failure when required.
src/vcpkg/commands.install.cpp	Uses the binary-cache upload result to influence the final exit code.
src/vcpkg/commands.ci.cpp	Uses the binary-cache upload result to fail CI runs when required.
src/vcpkg/commands.build.cpp	Returns failure on successful build if required uploads failed.
src/vcpkg/commands.upgrade.cpp	Fails the command when required uploads fail after upgrade processing.
src/vcpkg/commands.set-installed.cpp	Fails the command when required uploads fail after set-installed processing.
src/vcpkg/commands.test-features.cpp	Fails the command when required uploads fail even if feature tests passed.
src/vcpkg-test/arguments.cpp	Adds argument parsing tests for the new switch and its --no- form.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

[Billy O'Neal (BillyONeal)]

1. The command suggests that we should stop/terminate if an upload fails, but it seems the only thing you're actually doing is changing the exit code... but I also don't have better name suggestions right now so I can't really suggest that that should be a blocker.
2. Why can't the CI system you're trying to address with this change look for the error: ?
3. Should we distinguish between "all uploads to a given endpoint failed" and "an upload to a given endpoint failed" given that the former suggests a configuration problem while the latter may be just general network instability about which a build can do nothing.
4. If/when we do "inclusive" caches like #1597 how do you think that should work with this?
5. It might be a good idea to do the audit for making the console output for binary caching clearer similar to that requested in #1864 before trying to change the exit code. I'm concerned about "the exit code is nonzero... WHY?!" being a later user's reaction.

[xiaozhuai (xiaozhuai)]

it seems the only thing you're actually doing is changing the exit code

Yes, that's exactly what CI needs. When this happens, we need to be notified so we can trigger a rerun, rather than just marking everything as successful and causing everyone else's --only-binarycaching installs to fail.

but I also don't have better name suggestions right now

Coming up with a name is hard, isn't it? This name was first suggested by autoantwort
I thought about it for a while, but I couldn't come up with anything better.
If you have a name that conveys the meaning more clearly, I'd be happy to change it.

Why can't the CI system you're trying to address with this change look for the error: ?

This is very unreliable. We implement it by checking the logs for the message curl failed to PUT file because we use webdav backend. It worked fine at first, until we recently added a S3 backend.
The error log become

warning: /opt/homebrew/bin/aws s3 cp xxx.zip s3://xxx/xxx/xxxxxx.zip failed with exit code 1

Therefore, any attempt to match logs is unreliable.

Should we distinguish between "all uploads to a given endpoint failed" and "an upload to a given endpoint failed" given that the former suggests a configuration problem while the latter may be just general network instability about which a build can do nothing.

I don't think it's necessary—at least for my use case, which is a typical scenario where I build on the central CI and upload the binary cache, while using --only-binarycaching everywhere else. It's enough to know that the upload failed; all you need to do is trigger a rerun.

If it’s a configuration error, there’s nothing we can do.
If it’s a network issue—such as a temporary fluctuation—we usually just need to rerun CI. That's what's happening in real world. I admit this kind of fluctuation is rare, but when it does happen, everyone's installation fails—which is terrible.

If/when we do "inclusive" caches like #1597 how do you think that should work with this?

I believe that if the write-back fails and --require-binary-cache-upload is specified, a non-zero error code should also be returned.
Suppose a large organization has divided its binarycaching storage repositories by region to achieve the best user experience.

It might be a good idea to do the audit for making the console output for binary caching clearer similar to that requested in #1864 before trying to change the exit code. I'm concerned about "the exit code is nonzero... WHY?!" being a later user's reaction.

When the --require-binary-cache-upload option is not provided, the default behavior actually remains unchanged. I agree with you that the output should be modified to clearly inform users that the error is caused by a failed upload while the --require-binary-cache-upload option was specified.

[Copilot]

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.

[Copilot]

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.

[Billy O'Neal (BillyONeal)]

This is very unreliable. We implement it by checking the logs for the message curl failed to PUT file because we use webdav backend. It worked fine at first, until we recently added a S3 backend. The error log become

warning: /opt/homebrew/bin/aws s3 cp xxx.zip s3://xxx/xxx/xxxxxx.zip failed with exit code 1

Therefore, any attempt to match logs is unreliable.

Can we fix the output such that it would be reliable?

[xiaozhuai (xiaozhuai)]

Can we fix the output such that it would be reliable?

It's a workaround. Although this can work, I don't think it's better. Of course, if you insist, I'll update this PR.

[Copilot]

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

[Billy O'Neal (BillyONeal)]

Can we fix the output such that it would be reliable?

It's a workaround. Although this can work, I don't think it's better. Of course, if you insist, I'll update this PR.

I'm making it as a suggestion only because the "user visible" exposure is smaller and I think it's a net improvement for everyone rather than trying to do the particular thing you are trying to do.

Would you object to this being changed to fail/stop rather than continuing to build everything in the future?

[xiaozhuai (xiaozhuai)]

Would you object to this being changed to fail/stop rather than continuing to build everything in the future?

Yes, I do object to this change. I strongly prefer to keep the current 'continue building' approach.
If we really have to do that, then maybe we need another option to enable the fail-fast.

In fact, I don’t think a fail-fast strategy makes sense here.

Especially when upload tasks are actually performed asynchronously; if an upload fails, the current log has already rolled over to a later port in the build. This can be very confusing.

Another point is that for users who have configured multiple binary caching sources, one source might experience a temporary outage while others remain operational; in such cases, fail-fast is inappropriate. This will become clearer once the write-back feature is added to the system. The write-back feature works very well in conjunction with the options added in this PR.

[Billy O'Neal (BillyONeal)]

Yes, I do object to this change. I strongly prefer to keep the current 'continue building' approach.
If we really have to do that, then maybe we need another option to enable the fail-fast.

To me "require" means "fail immediately" and if you didn't object to that as a potential future change it was going to short circuit a lot of name bikeshedding.

How about --binary-cache-upload-exit-code or similar? Given that it only affects the exit code it would be nice to have something about that in there.

Also should this also apply to asset cache uploads? The systems are fairly similar.

[Victor Romero (vicroms)]

xiaozhuai (@xiaozhuai)

As an alternative to checking the exit code of vcpkg, the tool could produce a binary-cache-failure-log.txt file that only gets created/populated when a package upload fails. Then you could check for the existence of that file and fail the CI pipeline.

[xiaozhuai (xiaozhuai)]

To me "require" means "fail immediately"

I admit that "require" is misleading; I'll think of a better name.

Also should this also apply to asset cache uploads? The systems are fairly similar.

They do look very similar, however, I personally don't think it's necessary to add this option for it. Because the asset cache will automatically fall back to origin url. Even if it ultimately fails, the build will fail instead of quietly continuing as if everything were fine.

[xiaozhuai (xiaozhuai)]

As an alternative to checking the exit code of vcpkg, the tool could produce a binary-cache-failure-log.txt file that only gets created/populated when a package upload fails. Then you could check for the existence of that file and fail the CI pipeline.

IMHO, using exit code is more straightforward.

[xiaozhuai (xiaozhuai)]

How about replacing the boolean switch with a value-based option?
--binarycaching-upload-failure=<ignore|exit-nonzero>
ignore reports failures without affecting the exit code, which is the current behavior.
exit-nonzero would continue all builds and pending asynchronous uploads, then return a non-zero exit code if any upload failed.
This avoids implying fail-fast behavior and describes how upload failures are treated. A value-based option is also extensible: we could add fail-fast in the future without introducing another command-line option:
--binarycaching-upload-failure=fail-fast

[autoantwort]

for the bikeshedding: --exit-nonzero-if-upload-fails and then maybe also fail if asset cache upload fails.

[xiaozhuai (xiaozhuai)]

also fail if asset cache upload fails.

IMHO, I don't care about asset cache upload failures, because they don't actually have any side effects.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/vcpkg/binarycaching.cpp:2923

• The PR description says it adds --require-binary-cache-upload, tracks upload success/failure per destination, and propagates a nonzero exit code through multiple commands. In the current changes, there is no implementation or mention of this option/behavior (no occurrences of "require-binary-cache-upload" in src/ or include/), and the only functional change shown is adding a standardized diagnostic prefix in the background push thread. Either the description needs to be updated to match the actual scope, or the missing option/exit-code plumbing changes need to be included.

    BinaryCache::BinaryCache(const Filesystem& fs) : BinaryCache(fs, stdout_sink) { }
    BinaryCache::BinaryCache(const Filesystem& fs, MessageSink& message_sink)
        : m_fs(fs), m_bg_msg_sink(message_sink), m_push_thread(&BinaryCache::push_thread_main, this)

src/vcpkg/binarycaching.cpp:230

• BinaryCacheUploadDiagnosticContext only prefixes Error/Warning diagnostics. Upload failure paths often emit follow-up Note diagnostics (e.g., NuGet push reports a Note like "While packing NuGet package" after an error), which will now be emitted without the standardized "Binary cache upload failed" prefix, reducing the usefulness/consistency of the upload-failure diagnostic grouping.

            if (line.kind() == DiagKind::Error || line.kind() == DiagKind::Warning)
            {
                inner_context.report(DiagnosticLine{
                    line.kind(), msg::format(msgBinaryCacheUploadFailed).append_raw(": ").append(line.message_text())});
            }

[xiaozhuai (xiaozhuai)]

Reopen in #2105