Allowed to fail, asynchronous`repository_ctx.download` still reports as `pending download` after failure, swallowing the error

[ArchangelX360]

Description of the bug:

When using allow_fail = True and block = False, repository_ctx.download failed downloads:

• are reported incorrectly to still be pending
• not allowing the user to retrieve the actual error which is firing in allow_fail = False

Error in wait: java.io.IOException: Configured URL rewriter blocked all URLs: [https://raw.githubusercontent.com/tikv/jemallocator/baae33dd29ccd82c8330021418b293fa6eab5bbc/Cargo.toml]

Which category does this issue belong to?

No response

What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

1. Create a downloader config

# myconfig.cfg
block *

2. Set it as --downloader_config=myconfig.cfg
3. In a repository rule, run:

token = ctx.download(
    "https://raw.githubusercontent.com/tikv/jemallocator/baae33dd29ccd82c8330021418b293fa6eab5bbc/Cargo.toml",
    output = "Cargo.toml",
    allow_fail = True,
    block = False,
)
print(token)        # prints <pending download to '/Users/titouan.bion/Library/Caches/JetBrains/MonorepoBazel/aa71b1dc3fd25eda1f9f28ad27b5446f/modextwd/rules_rs++crate/Cargo.toml'>
print(token.wait()) # prints `struct(success = False)`
print(token)        # prints <pending download to '/Users/titouan.bion/Library/Caches/JetBrains/MonorepoBazel/aa71b1dc3fd25eda1f9f28ad27b5446f/modextwd/rules_rs++crate/Cargo.toml'>

Which operating system are you running Bazel on?

No response

What is the output of bazel info release?

No response

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

No response

What's the output of git remote get-url origin; git rev-parse HEAD ?

If this is a regression, please try to identify the Bazel commit where the bug was introduced with bazelisk --bisect.

No response

Have you found anything relevant by searching the web?

No response

Any other information, logs, or outputs that you want to share?

No response

[dzbarsky]

+1, it would be really nice to have some way to retrieve/introspect the resulting error when using allow_fail = True, I didn't see any way to get at that?

cc @fmeum in case you're interested :D

[fmeum]

I agree that being able to read the error of a failed download with allow_fail = True would be useful. Is that specific to block = False though? I would be slightly concerned about users starting to reason about these error messages via substrings, which could end up being very brittle.

The print behavior of download tokens is misleading, but it's currently really just a static string. We can make it more helpful.

Is there anything else specific to block = False that you think is not ideal?

[dzbarsky]

I agree that being able to read the error of a failed download with allow_fail = True would be useful. Is that specific to block = False though? I would be slightly concerned about users starting to reason about these error messages via substrings, which could end up being very brittle.

Sure, perhaps the token could have a print_failure() methor so it's opaque but not completely silent?

You're right that it's not specific to non-blocking, it's just that non-blocking makes things trickier to debug because it introduces the potential of failure due to race.

The print behavior of download tokens is misleading, but it's currently really just a static string. We can make it more helpful.

That would be swell. Between these 2 issues the behavior we saw was "token is pending. But we thought we called wait(). Is there a race? Nope, no race. Oh. Calling wait() does not actually block until it resolves, it returns still pending. Bazel bug?? And we had to go patch the rules to not have allow_fail to get the actual error, which is blocked download :)

Basically anything we can do to make the above failure less secret would be great!

Is there anything else specific to block = False that you think is not ideal?

It might also be nice to hang the download path off the token so users don't need to associate them manually?