Fix overflowing integer when retrieving comments for FCP

[apiraino]

This is a patch I've been using locally since long, should be the final nail in the coffin to fix the overflowing GH ID issue we started experiencing a while ago.

I'd like to upstream it after some testing.

[Mark-Simulacrum]

Might want to switch to u64 to preempt another round of problems.

[apiraino]

Note for myself to investigate before merge: using u64 causes a runtime error when retrieving issues

2025-02-26T16:29:55.830555Z DEBUG triagebot::github: send_req with RequestBuilder { method: GET, url: Url { scheme: "https", cannot_be_a_base: false, username: "", password: None, host: Some(Domain("api.github.com")), port: None, path: "/search/issues", query: Some("q=state:open+no:assignee+label:T-compiler+label:P-critical+repo:rust-lang/rust&sort=created&order=asc&per_page=100&page=1"), fragment: None }, headers: {"user-agent": "rust-lang-triagebot", "authorization": Sensitive} }
...
Error: out of range integral type conversion attempted

The error happens here and it's due to parsing the JSON that goes into IssueSearchResult.issues which is a Vec<Issue>, so there must be some integer casting that fails inside there

[Urgau]

@apiraino I don't know how changes to rfcbot structs could be related to a failure de-serializing IssueSearchResult in GitHub, are you sure they are related, may have been spurious failures.

[apiraino]

I can consistently reproduce the issue in my tests (which, due to GH 429'ing me, are pretty annoying)

[apiraino]

Finally I think this should be fixed, now we use u64 where needed. I didn't touch other IDs because I want to scope this PR on this specific problem. If in future we have other interegers overflows from GH, we will cope with them.

I also removed some code that was doing some dance with casts because it's not needed anymore

[apiraino]

Who wants to review? :)

r?

[rustbot]

Error: Parsing assign command in comment failed: ...'' | error: specify user to assign to at >| ''...

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

[Kobzol]

I'm not sure how does it help if RFCbot still serializes these fields as i32 (even if it converts them to u32 right before writing them, but that doesn't help if the value is over 2.something billion already on the rfcbot side), but it also hopefully shouldn't hurt, so let's see :)

Seems like in https://rfcbot.rs/api/all, some fk_initiating_comment values are e.g. 2669499833, so clearly they go over i32. Not sure how that's even possible, probably the values are actually represented as negative numbers in rfcbot or something? xD Anyway.

[apiraino]

I'm not sure how does it help if RFCbot still serializes these fields as i32 (even if it converts them to u32 right before writing them, but that doesn't help if the value is over 2.something billion already on the rfcbot side), but it also hopefully shouldn't hurt, so let's see :)

Seems like in rfcbot.rs/api/all, some fk_initiating_comment values are e.g. 2669499833, so clearly they go over i32. Not sure how that's even possible, probably the values are actually represented as negative numbers in rfcbot or something? xD Anyway.

I don't remember exactly the details anymore but some of things you mention were probably also what I was thinking at the time (IIUC your comment): rust-lang/rfcbot-rs#325

Would it make sense to close now rfcbot-rs#325? 🤔

Zulip discussion for the record (so we don't completely forget)
#t-infra > Integer overflow in RfcBot responses? @ 💬

[Kobzol]

I think that the fundamental issue is still that the IDs are represented as i32 in RFCbot's DB, rather than i64, and that issue is not solved yet. It seems very scary to perform migrations on RFCbot's DB 🥶 But we could try, I guess.