Retry 429 on every request, 502 on reads only

Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "corgea"
-version = "1.14.0"
+version = "1.14.1"
 edition = "2021"
 
 # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

README.md

@@ -48,20 +48,36 @@ evaluated; override with `CORGEA_BLOCKING_RULES_TIMEOUT_SECONDS`.
 trips: both are written before `--fail`/`--block-on` are evaluated, so a scan
 that exits 1 on a blocking rule still leaves its report behind to ingest.
 
-### Gateway errors are retried, not surfaced
-
-Every Corgea API call the CLI makes — uploads included — replays itself when the
-platform's proxy answers `502 Bad Gateway`, waiting 10s, then 30s, then 50s. A
-request that is still answered 502 after those three retries fails the command
-in the usual way, so a pipeline exits non-zero on a real outage and rides out the
-blips a busy platform produces under parallel scans. Each retry is logged, and
-the count belongs to a single request: any successful call starts the next one
-with the full three retries again.
-
-The one place this stops early is `corgea upload`'s per-file source upload: a 502
-that outlives the retries there is taken as the platform being unavailable rather
-than one bad file, so the remaining paths are reported as unsent instead of each
-spending another 90 seconds.
+### 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
+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
+way, so a real outage still exits non-zero. Each retry is logged, and the count
+belongs to a single request: any other answer starts the next one with the full
+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:
+
+- 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.
+
+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
+file, and reports the remaining paths as unsent instead of collecting the same
+answer once per path.
 
 ### Skipping a re-scan of the same commit
 

src/scan.rs

@@ -3,6 +3,7 @@ use crate::log::debug;
 use crate::scanners::parsers::ScanParserFactory;
 use crate::{utils, Config};
 use reqwest::header;
+use reqwest::Method;
 use serde_json::Value;
 use std::collections::HashSet;
 use std::io::{self, Read};
@@ -328,7 +329,7 @@ pub fn upload_scan(
     let mut uploaded_paths = HashSet::new();
     let mut uploaded_count = 0;
     let mut upload_error_count = 0;
-    let mut gateway_gave_up = false;
+    let mut platform_declined = false;
 
     'files: for path in &paths {
         if !Path::new(&path).exists() {
@@ -355,7 +356,7 @@ pub fn upload_scan(
 
         while attempts < 3 && !success {
             debug(&format!("POST: {}", src_upload_url));
-            let res = utils::api::send_with_retries("a source file upload", || {
+            let res = utils::api::send_with_retries("a source file upload", &Method::POST, || {
                 let form = reqwest::blocking::multipart::Form::new()
                     .file("file", fp)
                     .expect("Failed to read file");
@@ -373,18 +374,16 @@ pub fn upload_scan(
                             "Code upload failed with status: {}. Response body: {}",
                             status, body
                         ));
-                        // A 502 that outlived the retry schedule is the platform
-                        // being unavailable, not this one file. Retrying it here
-                        // would spend the schedule twice over, and walking the
-                        // remaining paths would spend a fresh 90 seconds on each
-                        // of them, so stop uploading source files altogether.
-                        if utils::api::is_gateway_error(status) {
-                            log::warn!(
-                                "Failed to upload file {} after the gateway retries: {}",
-                                path,
-                                status
-                            );
-                            gateway_gave_up = true;
+                        // 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
+                        // just collect the same answer once per file, so stop
+                        // uploading source files altogether.
+                        if utils::api::is_transient_error(status) {
+                            log::warn!("Failed to upload file {}: {}", path, status);
+                            platform_declined = true;
                             break 'files;
                         }
                         log::warn!("Failed to upload file {} {}... retrying", status, path);
@@ -419,12 +418,12 @@ pub fn upload_scan(
 
     // Everything the aborted walk never attempted still counts as unsent, or
     // the closing summary would report one failure for a whole skipped tree.
-    if gateway_gave_up {
+    if platform_declined {
         let distinct: HashSet<&String> = paths.iter().collect();
         let unsent = distinct.len() - uploaded_paths.len();
         upload_error_count += unsent;
         log::warn!(
-            "Stopped uploading source files: Corgea was still answering 502 after the retries. {} of {} files were not sent.",
+            "Stopped uploading source files: Corgea is not accepting them right now. {} of {} files were not sent.",
             unsent,
             distinct.len()
         );
@@ -468,15 +467,16 @@ pub fn upload_scan(
                 index + 1,
                 total_chunks
             ));
-            let response = utils::api::send_with_retries("a scan report chunk upload", || {
-                client
-                    .post(&scan_upload_url)
-                    .header(header::CONTENT_TYPE, "application/json")
-                    .header("Upload-Offset", offset.to_string())
-                    .header("Upload-Length", input_size.to_string())
-                    .body(chunk.to_vec())
-                    .send()
-            });
+            let response =
+                utils::api::send_with_retries("a scan report chunk upload", &Method::POST, || {
+                    client
+                        .post(&scan_upload_url)
+                        .header(header::CONTENT_TYPE, "application/json")
+                        .header("Upload-Offset", offset.to_string())
+                        .header("Upload-Length", input_size.to_string())
+                        .body(chunk.to_vec())
+                        .send()
+                });
 
             let should_break = match &response {
                 Ok(res) => {
@@ -519,7 +519,7 @@ pub fn upload_scan(
         last_response.expect("Failed to upload scan.")
     } else {
         debug(&format!("POST: {}", scan_upload_url));
-        utils::api::send_with_retries("the scan report upload", || {
+        utils::api::send_with_retries("the scan report upload", &Method::POST, || {
             client
                 .post(&scan_upload_url)
                 .header(header::CONTENT_TYPE, "application/json")
@@ -600,7 +600,7 @@ pub fn upload_scan(
     if git_config_path.exists() {
         debug("Uploading .git/config");
         debug(&format!("POST: {}", git_config_upload_url));
-        let res = utils::api::send_with_retries("the git config upload", || {
+        let res = utils::api::send_with_retries("the git config upload", &Method::POST, || {
             let form = reqwest::blocking::multipart::Form::new()
                 .file("file", git_config_path)
                 .expect("Failed to read file");