diff --git a/Cargo.lock b/Cargo.lock index 8298322..6fca48f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -369,7 +369,7 @@ checksum = "773648b94d0e5d620f64f280777445740e61fe701025087ec8b57f45c791888b" [[package]] name = "corgea" -version = "1.16.0" +version = "1.16.1" dependencies = [ "chrono", "clap", diff --git a/Cargo.toml b/Cargo.toml index 37d77a3..55a82d3 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "corgea" -version = "1.16.0" +version = "1.16.1" edition = "2021" # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html diff --git a/README.md b/README.md index 7dd3d6c..4dc95d1 100644 --- a/README.md +++ b/README.md @@ -50,8 +50,9 @@ that exits 1 on a blocking rule still leaves its report behind to ingest. ### Rate limits and gateway errors are retried -Neither `429 Too Many Requests` nor `502 Bad Gateway` is Corgea rejecting a -request on its merits, so both are retried rather than failed: the CLI waits +A `429 Too Many Requests`, a `502 Bad Gateway` or `504 Gateway Timeout`, and +Cloudflare's `520`–`524` are not Corgea rejecting a request on its merits, so +all of them are retried rather than failed: the CLI waits 10s, then 30s, then 50s, and a pipeline rides out the blips a busy platform produces under parallel scans instead of failing on them. A request still answered the same way after those three retries fails the command in the usual @@ -61,21 +62,23 @@ three retries again. A `429` that names a `Retry-After` in seconds is honored, up to two minutes for any one pause, and never shortens the pause below the schedule. -Which requests get retried depends on which of the two it is: +Which requests get retried depends on which it is: - A `429` retries everything, `POST` and `PATCH` included. The rate limiter declines the request before the API sees it, so nothing was created and sending it again finishes the same work. -- A `502` retries reads only. It comes from the proxy rather than from Corgea, - so it is equally the answer for "the request never arrived" and for "the - request was processed and the reply was lost coming back" — and every write - the CLI sends creates something, so re-sending one does not finish the first - scan, it starts a second. A write's 502 goes straight to the caller. +- A gateway error — `502`, `504`, or Cloudflare's `520`–`524` — retries reads + only. It comes from a proxy rather than from Corgea, so it is not Corgea's + word on what happened to the request: a `524` in particular means Corgea took + the request and was still working on it when Cloudflare stopped waiting. + Every write the CLI sends creates something, so re-sending one does not + finish the first scan, it starts a second. A write's gateway error goes + straight to the caller. Writes still retry network errors, where nothing reached Corgea at all. -`corgea upload` treats either status on a source upload — a 502, or a rate limit -that outlived its retries — as the platform being unavailable rather than one bad +`corgea upload` treats any of these on a source upload — a gateway error, or a +rate limit that outlived its retries — as the platform being unavailable rather than one bad file, and reports the remaining paths as unsent instead of collecting the same answer once per path. diff --git a/src/scan.rs b/src/scan.rs index 3823ff8..1e7b0e3 100644 --- a/src/scan.rs +++ b/src/scan.rs @@ -418,11 +418,12 @@ pub fn upload_scan( "Code upload failed with status: {}. Response body: {}", status, body )); - // A 502 or a rate limit that got this far is the - // platform being unavailable, not something wrong with - // this one file: the 502 because an upload is a write - // and so is never replayed, the 429 because its retries - // are already spent. Walking the remaining paths would + // A gateway error or a rate limit that got this far is + // the platform being unavailable, not something wrong + // with this one file: a gateway error such as a 502 or + // 524 because an upload is a write and so is never + // replayed, the 429 because its retries are already + // spent. Walking the remaining paths would // just collect the same answer once per file, so stop // uploading source files altogether. if utils::api::is_transient_error(status) { diff --git a/src/utils/api.rs b/src/utils/api.rs index a31b547..f971130 100644 --- a/src/utils/api.rs +++ b/src/utils/api.rs @@ -202,45 +202,71 @@ impl DebugRequestBuilder { } } -/// Statuses the CLI answers with a retry rather than a failure. +/// Cloudflare's own statuses for failing to get an answer from the origin. +/// They have no `StatusCode` constants because no RFC defines them. +const CLOUDFLARE_UNKNOWN_ERROR: u16 = 520; +const CLOUDFLARE_WEB_SERVER_DOWN: u16 = 521; +const CLOUDFLARE_CONNECTION_TIMED_OUT: u16 = 522; +const CLOUDFLARE_ORIGIN_UNREACHABLE: u16 = 523; +const CLOUDFLARE_TIMEOUT_OCCURRED: u16 = 524; + +/// Statuses where a proxy in front of the API failed to get an answer from it. /// -/// A `429` is Corgea's rate limiter turning the request away and a `502` is the -/// proxy in front of Corgea failing to get an answer from the API. Neither is -/// the API rejecting the request on its merits, so under the parallel scanning -/// load that produces them a retry succeeds where failing the pipeline would -/// not. -pub fn is_transient_error(status: StatusCode) -> bool { +/// - `502 Bad Gateway` and `504 Gateway Timeout` from any proxy. +/// - `520 Unknown Error`: the origin reset the connection or sent back +/// something that was not an HTTP response. +/// - `521 Web Server Is Down`: the origin refused Cloudflare's connection. +/// - `522 Connection Timed Out`: covers the connection never opening, but also +/// the request going out and never being acknowledged. +/// - `523 Origin Is Unreachable`: Cloudflare found no route to the origin. +/// - `524 A Timeout Occurred`: Cloudflare delivered the request and the origin +/// did not answer within the proxy read timeout (125s by default) — which +/// is the API still working on it, not the API never seeing it. +fn is_gateway_error(status: StatusCode) -> bool { matches!( status, - StatusCode::TOO_MANY_REQUESTS | StatusCode::BAD_GATEWAY + StatusCode::BAD_GATEWAY | StatusCode::GATEWAY_TIMEOUT + ) || matches!( + status.as_u16(), + CLOUDFLARE_UNKNOWN_ERROR + | CLOUDFLARE_WEB_SERVER_DOWN + | CLOUDFLARE_CONNECTION_TIMED_OUT + | CLOUDFLARE_ORIGIN_UNREACHABLE + | CLOUDFLARE_TIMEOUT_OCCURRED ) } +/// Statuses the CLI answers with a retry rather than a failure. +/// +/// A `429` is Corgea's rate limiter turning the request away and a gateway +/// error is a proxy in front of Corgea failing to get an answer from the API. +/// Neither is the API rejecting the request on its merits, so under the +/// parallel scanning load that produces them a retry succeeds where failing the +/// pipeline would not. +pub fn is_transient_error(status: StatusCode) -> bool { + status == StatusCode::TOO_MANY_REQUESTS || is_gateway_error(status) +} + /// Whether `status` is worth sending this `method` again. /// -/// Both transient statuses are worth retrying, but they promise different -/// things about what happened to the request, and only one of them is safe to -/// answer by re-sending a write: +/// Every transient status is worth retrying, but they promise different things +/// about what happened to the request, and only one is safe to answer by +/// re-sending a write: /// /// - `429 Too Many Requests` is the rate limiter declining the request before /// the API sees it. Nothing was created, so any method can be sent again — /// which matters most for the writes, since a rate-limited upload that failed /// the command leaves a pipeline to be re-run by hand. -/// - `502 Bad Gateway` comes from the proxy, so it says nothing about whether -/// the API acted: it is equally the answer for "never arrived" and for "was -/// processed, and the reply was lost coming back". Only a request that changes -/// nothing is safe to send into that ambiguity, and every write the CLI sends -/// creates something. `POST /start-scan` mints a transfer, `POST /scan-upload` -/// takes a whole report, and the archive `PATCH` that fills the last of -/// `Upload-Length` is the one that answers with `scan_id` — on an archive -/// under `CHUNK_SIZE`, the only chunk. Re-sending one of those does not finish -/// the first scan, it starts a second. +/// - A gateway error comes from a proxy, so it is not the API's word on what +/// happened to the request. Only a request that changes nothing is sent +/// again, because every write the CLI sends creates something. +/// `POST /start-scan` mints a transfer, `POST /scan-upload` takes a whole +/// report, and the archive `PATCH` that fills the last of `Upload-Length` is +/// the one that answers with `scan_id` — on an archive under `CHUNK_SIZE`, +/// the only chunk. Re-sending one of those does not finish the first scan, +/// it starts a second. fn should_retry(status: StatusCode, method: &Method) -> bool { - match status { - StatusCode::TOO_MANY_REQUESTS => true, - StatusCode::BAD_GATEWAY => method.is_safe(), - _ => false, - } + status == StatusCode::TOO_MANY_REQUESTS || (is_gateway_error(status) && method.is_safe()) } /// Pauses before each retry: a transient error has to survive four attempts @@ -2982,22 +3008,23 @@ mod tests { Duration::from_secs(50) ] ); - assert!(is_transient_error(StatusCode::BAD_GATEWAY)); - assert!(is_transient_error(StatusCode::TOO_MANY_REQUESTS)); + for code in [429, 502, 504, 520, 521, 522, 523, 524] { + assert!(is_transient_error(status(code)), "{code}"); + } // Everything else is the API answering for itself, including the other // 5xx: those are not what the pipelines are hitting, and replaying an - // upload the server did read is not free. - for status in [ - StatusCode::OK, - StatusCode::UNAUTHORIZED, - StatusCode::NOT_FOUND, - StatusCode::INTERNAL_SERVER_ERROR, - StatusCode::SERVICE_UNAVAILABLE, - ] { - assert!(!is_transient_error(status), "{status}"); + // upload the server did read is not free. 525, 526 and 530 are + // Cloudflare too, but a broken certificate or DNS record does not fix + // itself within the schedule. + for code in [200, 401, 404, 500, 503, 525, 526, 530] { + assert!(!is_transient_error(status(code)), "{code}"); } } + fn status(code: u16) -> StatusCode { + StatusCode::from_u16(code).unwrap() + } + #[test] fn retry_delay_override_falls_back_to_the_schedule_when_unusable() { assert_eq!(parse_retry_delays(None), TRANSIENT_RETRY_DELAYS.to_vec()); @@ -3105,6 +3132,72 @@ mod tests { } } + #[test] + fn cloudflare_gateway_errors_are_retried_only_for_reads() { + // A proxy's word on the request, not the API's: only reads go again. + for code in [504, 520, 521, 522, 523, 524] { + for method in [Method::GET, Method::HEAD] { + assert!(should_retry(status(code), &method), "{code} {method}"); + } + for method in [Method::POST, Method::PUT, Method::PATCH, Method::DELETE] { + assert!( + !should_retry(status(code), &method), + "{code} {method} must be sent once" + ); + } + } + for code in [525, 526, 530] { + assert!(!should_retry(status(code), &Method::GET), "{code}"); + } + } + + #[test] + fn send_retries_a_cloudflare_timeout_on_a_read() { + let (base, hits) = spawn_failing_stub( + 2, + "524 A Timeout Occurred", + "", + "error code: 524", + ); + + let response = http_client().get(&base).send().expect("send"); + + assert_eq!(response.status(), StatusCode::OK); + assert_eq!(hits.load(Ordering::SeqCst), 3, "two 524s, then the answer"); + } + + #[test] + fn send_leaves_a_post_at_one_attempt_on_a_cloudflare_timeout() { + // The origin took the request and was still working on it when + // Cloudflare gave up; sending it again would start a second scan. + let (base, hits) = spawn_failing_stub( + usize::MAX, + "524 A Timeout Occurred", + "", + "error code: 524", + ); + + let response = http_client().post(&base).body("{}").send().expect("send"); + + assert_eq!(response.status().as_u16(), 524); + assert_eq!(hits.load(Ordering::SeqCst), 1); + } + + #[test] + fn send_leaves_a_post_at_one_attempt_when_the_origin_is_down() { + let (base, hits) = spawn_failing_stub( + usize::MAX, + "521 Web Server Is Down", + "", + "error code: 521", + ); + + let response = http_client().post(&base).body("{}").send().expect("send"); + + assert_eq!(response.status().as_u16(), 521); + assert_eq!(hits.load(Ordering::SeqCst), 1); + } + #[test] fn the_schedule_stops_after_its_retries_and_any_other_answer_resets_it() { let mut retries = TransientRetries::new("GET /api/v1/scan/s1");