AWS S3 cache miss should not issue warning

[petamas]

This fixes microsoft/vcpkg#31932

Previously, when using the AWS S3 binary cache, every cache miss triggered the following message:

warning: error: aws failed with exit code: (1).

This was both noisy and misleading (because it was not an actual error).

Now AWS cache misses are treated silently, the same way as cache misses from the local cache are.

Note

I don't have access to either Google Cloud Storage or Tencent Cloud Object Storage sources, so I don't know whether the same kind of issue happens there too; in any case, fixing those should be easy using the newly introduced interface. (Their stat() and download_file() methods need to be adapted to return CacheResult::miss in the relevant cases.)

[petamas]

@microsoft-github-policy-service agree company="Formlabs"

[BillyONeal]

Can this use RestoreResult instead of making up a new value?

[petamas]

Absolutely, I just didn't find that enum.

[BillyONeal]

Should stat return CacheAvailability instead?

[petamas]

Do you mean ExpectedL<CacheAvailability>, or plain CacheAvailability? In either case, how should we treat unknown in the call sites?

[petamas]

OK, I've found your suggestion for precheck(), but it's still unclear to me how to handle it in AwsStorageTool::download_file(). Should I simply return RestoreResult:: unavailable?

[BillyONeal]

Do you mean ExpectedL<CacheAvailability>, or plain CacheAvailability? In either case, how should we treat unknown in the call sites?

Sorry, I meant ExpectedL<CacheAvailability> :)

[BillyONeal]

I think this value_or is less clear than just spelling it out:

if (auto cache_result = res.get()) {
    if (*cache_result == CacheResult::hit) {
        out_zip_paths[idx].emplace(std::move(tmp), RemoveWhen::always);
        return;
    }
} else {
    out_sink.println_warning(res.error());
}

[BillyONeal]

If stat returns CacheAvailability then this can be:

auto maybe_res = m_tool->stat(make_object_path(m_prefix, abi));
if (auto res = maybe_res.get()) {
    cache_status[idx] = *res;
}

[BillyONeal]

Is this a case where we should be looking at the output to confirm we are in the case we expect?

    ExpectedL<CacheResult> stat_flatten_pseudocode(const ExpectedL<ExitCodeAndOutput>& maybe_exit, StringView tool_name)
    {
        if (auto exit = maybe_exit.get())
        {
            if (exit->exit_code == 0)
            {
                return {CacheResult::hit};
            }
            if (exit->exit_code == 1 && exit->output.empty())
            {
                return {CacheResult::miss};
            }

            return {msg::format_error(
                        msgProgramReturnedNonzeroExitCode, msg::tool_name = tool_name, msg::exit_code = exit->exit_code)
                        .append_raw('\n')
                        .append_raw(exit->output)};
        }

        return {msg::format_error(msgLaunchingProgramFailed, msg::tool_name = tool_name)
                    .append_raw(' ')
                    .append_raw(maybe_exit.error().to_string())};
    }

(this would of course remove the need for unit_to_cache_hit)

I'm a bit nervous to treat all potentially failing exit codes as cache miss. For example, if there's an authentication failure, that probably prints something about what happened or similar, and probably also returns EXIT_FAILURE. But with this PR's suggestion that cache provider would be silently broken.

[petamas]

I think you might've misread my code. I also wouldn't want to treat all nonzero exit codes as cache misses, that's why I only changed the behaviour in the AWS provider, and only in case the output is empty, and the exit code is 0 or 1.

This specifically is the GCS provider, for which I tried to keep the original behaviour: it's either a cache hit, or an error with an error message. flatten() treats every nonzero exit code as an error, and returns a "right-filled" Expected with an error message, which I return as-is. Zero exit codes result in a Unit, which I treat as a hit like before. This function cannot return a cache miss at all, because I don't know what the right condition for that is in GCS caches.

Your function is almost equivalent to my AWS stat version, except that I treat "0 exit code, but empty stdout" as a cache miss, to handle if AWS maintainers decide to change aws ls behaviour to not return an error when there are no hits. However, I don't know how GCS reacts in the "not found" case, so I didn't want to change the behaviour from what it was before.

[BillyONeal]

I see. Yes, I was reviewing what this was trying to do based on what your PR description said and I didn't notice that this wasn't the AWS one. (Or if I did notice, I didn't internalize the difference...)

GitHub only showing like 2 lines of context == review pain :(

[BillyONeal]

Ditto stat_flatten_pseudocode above

[BillyONeal]

Thanks for the fix!

[petamas]

Thanks for your detailed review! I'll address your comments in a couple days. I added some questions in replies where something was unclear to me.

[petamas]

@BillyONeal
What's your preference on changes addressing review comments? Should I force-push the branch to rewrite history with the "correct" version incorporating the changes, or should I simply add my changes on top of the current ones in a separate commit?

[BillyONeal]

@BillyONeal What's your preference on changes addressing review comments? Should I force-push the branch to rewrite history with the "correct" version incorporating the changes, or should I simply add my changes on top of the current ones in a separate commit?

There isn't widespread surgery here so I think you can do whatever you want :)

[petamas]

@BillyONeal: I've updated my branch. I split the changes into two commits:

• The first one does not change the behaviour at all, only introduces the ability to return CacheAvailability/RestoreResult, but all providers always either return available or error, just like before.
• The second one makes AWS's stat() return unavailable instead of an error in case the standard output is empty, and the exit code is either 1 (current behaviour of aws-cli) or 0 (possible future behaviour if they decide the address this issue).

If you have a better name for flatten_generic(), I'm open to suggestions.

[petamas]

Just realized that this is not an equivalent transformation: if stat() returns an error, the original version sets cache_status[idx] to CacheAvailability::unavailable, while the new version leaves it as CacheAvailability::unknown. Should I add an else branch to keep the behaviour the same as before? (My understanding is that leaving it unknown may at worst cause an extra query to the provider, but I'm not 100% sure I'm reading the rest of the code right.)

[petamas]

GitHub's not showing the diff properly.

Original:

                if (m_tool->stat(make_object_path(m_prefix, abi)))
                {
                    cache_status[idx] = CacheAvailability::available;
                }
                else
                {
                    cache_status[idx] = CacheAvailability::unavailable;
                }

Current:

                auto maybe_res = m_tool->stat(make_object_path(m_prefix, abi));
                if (auto res = maybe_res.get())
                {
                    cache_status[idx] = *res;
                }

Possible fix to keep old behaviour:

                auto maybe_res = m_tool->stat(make_object_path(m_prefix, abi));
                if (auto res = maybe_res.get())
                {
                    cache_status[idx] = *res;
                }
                else
                {
                    cache_status[idx] = CacheAvailability::unavailable;
                }

[petamas]

I decided to go with the real equivalent change (i.e. I readded the else branch), and force-pushed.