Fix too many 429 response

[NobodyXu]

Fixed #1229

[NobodyXu]

@CAD97 @halostatue Can you try this PR if you have time?
Thanks!

[NobodyXu]

You can find the pre-built binaries in CI once it's done.

[halostatue]

I had previously force-upgraded some of these tools, but the aarch64 version (I’m on macOS 13 with an M1) is reporting a different issue now:

$ cargo-binstall cargo-info
 INFO resolve: Resolving package: 'cargo-info'
ERROR Fatal error:
  × For crate cargo-info: Failed to parse target triple: Unrecognized architecture: x86_64h
  ├─▶ Failed to parse target triple: Unrecognized architecture: x86_64h
  ╰─▶ Unrecognized architecture: x86_64h

I’m not sure that this is a useful test because the error is so different.

[NobodyXu]

I had previously force-upgraded some of these tools, but the aarch64 version (I’m on macOS 13 with an M1) is reporting a different issue now:

$ cargo-binstall cargo-info

 INFO resolve: Resolving package: 'cargo-info'

ERROR Fatal error:

  × For crate cargo-info: Failed to parse target triple: Unrecognized architecture: x86_64h

  ├─▶ Failed to parse target triple: Unrecognized architecture: x86_64h

  ╰─▶ Unrecognized architecture: x86_64h

I’m not sure that this is a useful test because the error is so different.

This is caused by #1228, we added support for a new target triple that target-lexicon doesn't have support yet.

I will open a PR tomorrow, meanwhile you can use --target to override the list of targets to probe.

[halostatue]

❯ cargo-binstall cargo-info --targets aarch64-apple-darwin
 INFO resolve: Resolving package: 'cargo-info'
 WARN The package cargo-info v0.7.6 will be downloaded from third-party source QuickInstall
 INFO This will install the following binaries:
 INFO   - cargo-info (cargo-info -> /Users/austin/.cargo/bin/cargo-info)
Do you wish to continue? yes/[no]
? yes
 INFO Installing binaries...
 INFO Done in 9.112601583s

Various caveats around not having done much since I uploaded the log (so the 429s may have been resolved) and having previously updated all of the tools, so it’s not a perfect test, but it seems to have worked.

One note, and it’s not exactly cargo-binstall's issue, but the integration with cargo-install-update seems to be incomplete as there appears to be no way to pass --targets to the cargo-binstall invocation.

[NobodyXu]

Various caveats around not having done much since I uploaded the log (so the 429s may have been resolved) and having previously updated all of the tools, so it’s not a perfect test, but it seems to have worked.

You can try passing --no-discover-github-token and does not explicitly pass GITHUB_TOKEN or GH_TOKEN via environment, then running cargo-binstall in a loop using bash script.

Without the github-token, binstall will hit github Restful API rate-limit and fallback to creating a http request instead, so it would probably trigger 429 rate-limit again.

One note, and it’s not exactly cargo-binstall's issue, but the integration with cargo-install-update seems to be incomplete as there appears to be no way to pass --targets to the cargo-binstall invocation.

It seems that cargo-install-update does not have a --target at all, you could request for one and also forward it to cargo-install.

[0atman]

I just ran target/release/cargo-binstall exa from main and this branch.
BOTH exhibit this bug.

deck@steamdeck ~/D/cargo-binstall (fix/too-many-429) [32]> target/release/cargo-binstall exa
 INFO resolve: Resolving package: 'exa'
 WARN Your GitHub API token (if any) has reached its rate limit and cannot be used again until Instant { tv_sec: 1690803842, tv_nsec: 588159616 }, so we will fallback to HEAD/GET on the url.
 WARN If you did not supply a github token, consider doing so: GitHub limits unauthorized users to 60 requests per hour per origin IP address.
 WARN Your GitHub API token (if any) has reached its rate limit and cannot be used again until Instant { tv_sec: 1690803842, tv_nsec: 588159616 }, so we will fallback to HEAD/GET on the url.
 WARN If you did not supply a github token, consider doing so: GitHub limits unauthorized users to 60 requests per hour per origin IP address.
^C WARN Installation cancelled

Did I build it wrong? I just cargo build --release and then ran the release executable, as above.

[0atman]

cc @NobodyXu

[NobodyXu]

I just ran target/release/cargo-binstall exa from main and this branch. BOTH exhibit this bug.

deck@steamdeck ~/D/cargo-binstall (fix/too-many-429) [32]> target/release/cargo-binstall exa
 INFO resolve: Resolving package: 'exa'
 WARN Your GitHub API token (if any) has reached its rate limit and cannot be used again until Instant { tv_sec: 1690803842, tv_nsec: 588159616 }, so we will fallback to HEAD/GET on the url.
 WARN If you did not supply a github token, consider doing so: GitHub limits unauthorized users to 60 requests per hour per origin IP address.
 WARN Your GitHub API token (if any) has reached its rate limit and cannot be used again until Instant { tv_sec: 1690803842, tv_nsec: 588159616 }, so we will fallback to HEAD/GET on the url.
 WARN If you did not supply a github token, consider doing so: GitHub limits unauthorized users to 60 requests per hour per origin IP address.
^C WARN Installation cancelled

Did I build it wrong? I just cargo build --release and then ran the release executable, as above.

It seems that your machine is still rate limited, but I do wonder how many warnings did you get?

We cannot fix the rate limit since it is imposed by GitHub, this PR is rather to reduce amount of warnings for rate-limit.

@0atman Can you run it again to see how many rate-limited warnings it produces please?

[NobodyXu]

Checking the CI, it seems that this PR does reduce the log, though I think there is still room to improve.

We could potentially increase the rate limit locally when hitting rate limit from server.

[NobodyXu]

I would like to replace tower::limit::RateLimit with our homebrew implementation that can increase per or num_requests freely without having to reset entire Service.

[NobodyXu]

Looking at the CI on windows it looks like it could still generate a lot of warnings logs for this, although I do think I need some review on how to proceed.

[NobodyXu]

Aha the log comes from binstalk for falling back to simply getting the url instead of using GhApiClient, I could do something about that.

[NobodyXu]

CI shows that this PR is working and it should indeed cut the amount of excessive log to just a few of them and no longer disturbing.

cc @CAD97 @halostatue @0atman

[CAD97]

I tested this commit (07cdc92) and I get 20 info logs for the 429, which is still a bit surprising, but is significantly improved from the 100+ dropped into the terminal simultaneously. And this is with supplying a gh token, fwiw.

cargo binstall wasm-bindgen-cli --github-token (gh auth token) --log-level debug | save binstall.debug.log

binstall.debug.log

wasm-bindgen-cli (the crate that consistently hits the 429s) is an unfortunate edge case, because with a repository path of https://github.com/rustwasm/wasm-bindgen/tree/main/crates/cli and generating lookup paths like https://github.com/rustwasm/wasm-bindgen/tree/main/crates/cli/releases/download/0.2.87/wasm-bindgen-cli-x86_64-pc-windows-msvc.tar.gz, it's never going to find the actual release binary at https://github.com/rustwasm/wasm-bindgen/releases/download/0.2.87/wasm-bindgen-0.2.87-x86_64-pc-windows-msvc.tar.gz; instead of checking the github releases, it's checking paths in the source tree. And it's checking at least 2520 such paths, I think, based on grepping the log file.

[NobodyXu]

I tested this commit (07cdc92) and I get 20 info logs for the 429, which is still a bit surprising, but is significantly improved from the 100+ dropped into the terminal simultaneously. And this is with supplying a gh token, fwiw.

Thank you!

I will have a look once I get back to my laptop, since it's hard to open this on my phone.

Tips: You could put the log in release and fold it using html tag details like this:

Some very long logs.

wasm-bindgen-cli (the crate that consistently hits the 429s) is an unfortunate edge case, because with a repository path of https://github.com/rustwasm/wasm-bindgen/tree/main/crates/cli and generating lookup paths like https://github.com/rustwasm/wasm-bindgen/tree/main/crates/cli/releases/download/0.2.87/wasm-bindgen-cli-x86_64-pc-windows-msvc.tar.gz, it's never going to find the actual release binary at https://github.com/rustwasm/wasm-bindgen/releases/download/0.2.87/wasm-bindgen-0.2.87-x86_64-pc-windows-msvc.tar.gz; instead of checking the github releases, it's checking paths in the source tree. And it's checking at least 2520 such paths, I think, based on grepping the log file.

Actually binstall does support this kind of scenario, binstall parse the repo and extract subcrate path, though the extracted path isn't guaranteed to be the actual prefix for the release and we could still improve on that.

[NobodyXu]

@CAD97 I was able to open this on iPhone using notes, and I recommends you to uplod a debug log since it is more than enough, cargo-binstall (as far as I can remember) log all useful debug info in debug log-level.

When you turned on the tracing log level, it will also include logs from the dependencies, like rustls, trust-dns, making it much harder to read and makes sense of, and also potentially leaking your personal information, since rustls or reqwest logs your tls handshake (including random and session id) in the log, @CAD97 I actually suggest you to take the original log down and replace it with the debug logging.

[CAD97]

I do know how to use <details>, but the logfile is 125597 544857 lines which is a bit more than GH likes in a comment (understatement). (I let the second run go a little longer before stopping it, and actually had to trim the log by half to get it to the allowed filesize)

Actually binstall does support this kind of scenario

Doesn't seem to be handling wasm-bindgen-cli:

Resolved repo_info = RepoInfo {
    repo: Url {
        scheme: "https",
        cannot_be_a_base: false,
        username: "",
        password: None,
        host: Some(
            Domain(
                "github.com",
            ),
        ),
        port: None,
        path: "/rustwasm/wasm-bindgen/tree/main/crates/cli",
        query: None,
        fragment: None,
    },
    repository_host: GitHub,
    subcrate: None,
}

and this looks like because it's expecting /[owner]/[repo]/tree/[branch]/[subcrate] but wasm-bindgen is using /[owner]/[repo]/tree/[branch]/crates/[subcrate]. This seems to be the general recommendation.

potentially leaking your personal information, since rustls or reqwest logs your tls handshake (including random and session id)

I did scan for PII, but still a decent idea; did so. (Well, sent the ticket to remove the file, anyway, and deleted the comment revision linking it, though ofc your quote still links it until the file dies.)

[NobodyXu]

I do know how to use <details>, but the logfile is 125597 544857 lines which is a bit more than GH likes in a comment (understatement). (I let the second run go a little longer before stopping it, and actually had to trim the log by half to get it to the allowed filesize)

Oh I forgot that it's just too large for a comment.

and this looks like because it's expecting /[owner]/[repo]/tree/[branch]/[subcrate] but wasm-bindgen is using /[owner]/[repo]/tree/[branch]/crates/[subcrate]. This seems to be the general recommendation.

Thanks, I'd like to open a separate PR for this.

We used because the other crate we support uses this schema, I suppose that we can check for "crate" and pop another url path segment for crate-name.

Or perhaps we can use crate-name instead of whatever specified in the repo link.

I did scan for PII, but still a decent idea; did so. (Well, sent the ticket to remove the file, anyway, and deleted the comment revision linking it, though ofc your quote still links it until the file dies.)

I've edited my comment to remove it.

[NobodyXu]

@CAD97 I've updated this PR to fix the excessive "client side rate-limit" debug logging (it was, in fact, added in this PR), now it should be much less debug logging.

P.S. I can only see one line of info! logging from the log you provided, so I wasn't able to identify other 20-line rate-limit logging you mentioned.

[NobodyXu]

cc @CAD97 @halostatue @0atman @passcod I would like this PR to get a final review and merged if everything is good.

[passcod]

This is more of a rubber-stamp; I haven't deeply looked at this but it seems right and I trust you.

[0atman]

Sorry I dropped off this bug due to IRL issues - I updated to the latest version of binstall today and was delighted to find everything working! I thought it only right to come here and thank everyone who worked on it :-)

[halostatue]

Same here. I’m expecting to update today and I’m looking forward to this.