Bound and classify local version-manager network operations - #496
Conversation
sdairs
changed the title
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| let category = match status.as_u16() { | ||
| 403 => NetworkCategory::Forbidden, | ||
| 404 => NetworkCategory::NotFound, | ||
| 429 => NetworkCategory::RateLimited, | ||
| _ if status.is_server_error() => NetworkCategory::Server, | ||
| _ if status.is_client_error() => NetworkCategory::Client, | ||
| _ => NetworkCategory::UnexpectedStatus, | ||
| }; |
There was a problem hiding this comment.
🟠 High version_manager/network.rs:100
HTTP 408 responses are classified as Client, so download_with_retry treats transient request timeouts as non-retryable and aborts artifact installation immediately. Classify 408 as Timeout so is_retryable() applies the retry policy.
| let category = match status.as_u16() { | |
| 403 => NetworkCategory::Forbidden, | |
| 404 => NetworkCategory::NotFound, | |
| 429 => NetworkCategory::RateLimited, | |
| _ if status.is_server_error() => NetworkCategory::Server, | |
| _ if status.is_client_error() => NetworkCategory::Client, | |
| _ => NetworkCategory::UnexpectedStatus, | |
| }; | |
| let category = match status.as_u16() { | |
| 403 => NetworkCategory::Forbidden, | |
| 404 => NetworkCategory::NotFound, | |
| 408 => NetworkCategory::Timeout, | |
| 429 => NetworkCategory::RateLimited, | |
| _ if status.is_server_error() => NetworkCategory::Server, | |
| _ if status.is_client_error() => NetworkCategory::Client, | |
| _ => NetworkCategory::UnexpectedStatus, | |
| }; |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @crates/clickhousectl/src/version_manager/network.rs around lines 100-107:
HTTP 408 responses are classified as `Client`, so `download_with_retry` treats transient request timeouts as non-retryable and aborts artifact installation immediately. Classify 408 as `Timeout` so `is_retryable()` applies the retry policy.
| @@ -127,11 +129,20 @@ pub async fn list_available_versions_from_builds() -> Result<Vec<String>> { | |||
| for mm in (1..=12).rev() { | |||
| let version_path = format!("{}.{}", yy, mm); | |||
| let url = builds_probe_url(&version_path, &platform); | |||
| match client.head(&url).send().await { | |||
| Ok(resp) if resp.status().is_success() => { | |||
| match client.head(&url, NetworkStage::BuildsList).await { | |||
There was a problem hiding this comment.
🟠 High version_manager/list.rs:118
list_available_versions_from_builds returns a timeout instead of the available-version list when the 84 sequential HEAD probes exceed the single 30-second OperationClient deadline, even though the connection remains usable. Because the client is created once at line 119, every probe shares that deadline; create independent per-probe deadlines or give the full scan a deadline sized for all requests.
- let first_url = builds_probe_url("20.1", &platform);
- let client = OperationClient::metadata(NetworkStage::BuildsList, &first_url)?;
-
let current_year = chrono::Utc::now().year() as u32;
@@
let version_path = format!("{}.{}", yy, mm);
let url = builds_probe_url(&version_path, &platform);
+ let client = OperationClient::metadata(NetworkStage::BuildsList, &url)?;
match client.head(&url, NetworkStage::BuildsList).await {🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @crates/clickhousectl/src/version_manager/list.rs around lines 118-132:
`list_available_versions_from_builds` returns a timeout instead of the available-version list when the 84 sequential HEAD probes exceed the single 30-second `OperationClient` deadline, even though the connection remains usable. Because the client is created once at line 119, every probe shares that deadline; create independent per-probe deadlines or give the full scan a deadline sized for all requests.
| let future = Utc::now() + chrono::Duration::seconds(30); | ||
| let parsed = parse_retry_after(&future.to_rfc2822()).unwrap(); | ||
| assert!(parsed >= Duration::from_secs(28)); | ||
| assert!(parsed <= Duration::from_secs(30)); | ||
| } |
There was a problem hiding this comment.
🟡 Medium version_manager/network.rs:271
This test fails nondeterministically when the task is delayed for roughly two seconds between constructing future and calling parse_retry_after, because parsed then falls below the hard-coded 28-second lower bound. Use a fixed RFC 2822 input and a non-time-sensitive assertion instead.
- let future = Utc::now() + chrono::Duration::seconds(30);
- let parsed = parse_retry_after(&future.to_rfc2822()).unwrap();
- assert!(parsed >= Duration::from_secs(28));
- assert!(parsed <= Duration::from_secs(30));
+ let parsed = parse_retry_after("Thu, 01 Jan 2099 00:00:00 +0000").unwrap();
+ assert!(parsed > Duration::from_secs(60 * 60 * 24 * 365));🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @crates/clickhousectl/src/version_manager/network.rs around lines 271-275:
This test fails nondeterministically when the task is delayed for roughly two seconds between constructing `future` and calling `parse_retry_after`, because `parsed` then falls below the hard-coded 28-second lower bound. Use a fixed RFC 2822 input and a non-time-sensitive assertion instead.
There was a problem hiding this comment.
🟠 High
resolve_major returns an earlier highest_available minor after a later probe fails, so a timeout or HTTP 5xx while checking 25.11 causes 25.10 to be selected without checking 25.12. Only return the probed minor when no probe failure occurred; otherwise continue to the GitHub fallback or return the classified failure.
- if let Some(minor) = highest_available {
+ if let Some(minor) = highest_available.filter(|_| first_probe_failure.is_none()) {🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @crates/clickhousectl/src/version_manager/resolve.rs around line 164:
`resolve_major` returns an earlier `highest_available` minor after a later probe fails, so a timeout or HTTP 5xx while checking `25.11` causes `25.10` to be selected without checking `25.12`. Only return the probed minor when no probe failure occurred; otherwise continue to the GitHub fallback or return the classified failure.
| return Some(Duration::from_secs(seconds)); | ||
| } | ||
|
|
||
| let retry_at = DateTime::parse_from_rfc2822(value) |
There was a problem hiding this comment.
🟡 Medium version_manager/network.rs:154
parse_retry_after returns None for valid HTTP-date Retry-After headers in RFC 850 (Sunday, 06-Nov-94 08:49:37 GMT) or ANSI asctime (Sun Nov 6 08:49:37 1994) form. Because line 154 only tries parse_from_rfc2822, those responses are ignored and the retry policy falls back to its short exponential delay; parse both additional HTTP-date formats before returning None.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @crates/clickhousectl/src/version_manager/network.rs around line 154:
`parse_retry_after` returns `None` for valid HTTP-date `Retry-After` headers in RFC 850 (`Sunday, 06-Nov-94 08:49:37 GMT`) or ANSI `asctime` (`Sun Nov 6 08:49:37 1994`) form. Because line 154 only tries `parse_from_rfc2822`, those responses are ignored and the retry policy falls back to its short exponential delay; parse both additional HTTP-date formats before returning `None`.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 39457ac. Configure here.
| } | ||
|
|
||
| /// `install 25.12.9.61` — exact version, needs channel from GH API | ||
| async fn resolve_exact(version: &str, platform: &Platform) -> Result<ResolvedVersion> { |
There was a problem hiding this comment.
Major resolve drops confirmed builds
High Severity
resolve_major only returns a builds source when every probe succeeds. After some minors are marked Available, a later unexpected probe error still discards highest_available and forces GitHub fallback. If that fallback also fails, install errors even though a builds path was already confirmed.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 39457ac. Configure here.
| ) | ||
| .into()); | ||
| } | ||
| Err(error) => return Err(error.into()), |
There was a problem hiding this comment.
Builds list deadline too short
Medium Severity
list_available_versions_from_builds runs up to roughly seven years of monthly HEAD probes on one OperationClient::metadata client, which shares a single 30s total deadline. Moderate latency or a couple of slow responses exhausts that budget mid-scan and fails the whole local list instead of finishing the listing.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 39457ac. Configure here.
1 participant
Summary
Retry-Afterwithin a five-second backoff cap.Tests
cargo test -p clickhousectl version_manager(95 passed)cargo test -p clickhousectl(all passed)cargo fmt --all --checkcargo clippy -p clickhousectl --all-targets -- -D warningsStack
issue-463-parse-version-operands(Validate local ClickHouse version operands during CLI parsing #485)Closes #459