Treat all 403s as cancellation, regardless of Job-Status

gitlab-runner-mock/src/api/request.rs

@@ -8,11 +8,9 @@ use wiremock::{Request, Respond};
 use crate::{GitlabRunnerMock, MockJobState};
 
 /*
-  jobs/id => 200  if ok;
+  jobs/id => 200 if ok;
 
-  403 if cancelled
-  < job-status: canceled in header
-}
+  403 if job token is invalid or the job was cancelled
 */
 
 #[derive(Default, Deserialize)]

gitlab-runner-mock/src/api/trace.rs

@@ -53,19 +53,16 @@ impl Respond for JobTraceResponder {
         };
 
         if let Some(job) = self.mock.get_job(id) {
-            if token != job.token() {
+            if token != job.token() || job.state() != MockJobState::Running {
                 ResponseTemplate::new(StatusCode::FORBIDDEN)
-            } else if job.state() != MockJobState::Running {
-                ResponseTemplate::new(StatusCode::FORBIDDEN)
-                    .insert_header("Job-Status", &*job.state().to_string())
             } else {
                 match job.append_log(request.body.clone(), start, end) {
                     Ok(()) => ResponseTemplate::new(StatusCode::ACCEPTED)
                         .insert_header(
                             "X-GitLab-Trace-Update-Interval",
-                            &*self.mock.update_interval().to_string(),
+                            self.mock.update_interval().to_string(),
                         )
-                        .insert_header("Job-Status", &*job.state().to_string()),
+                        .insert_header("Job-Status", job.state().to_string()),
                     Err(e) => ResponseTemplate::new(StatusCode::RANGE_NOT_SATISFIABLE)
                         .set_body_string(format!("{e:?}")),
                 }

gitlab-runner-mock/src/api/update.rs

@@ -35,29 +35,22 @@ impl Respond for JobUpdateResponder {
             .unwrap();
 
         if let Some(job) = self.mock.get_job(id) {
-            if r.token != job.token() {
+            if r.token != job.token() || job.state() != MockJobState::Running {
                 ResponseTemplate::new(StatusCode::FORBIDDEN)
             } else {
-                let r = match (job.state(), r.state) {
-                    (MockJobState::Running, MockJobState::Success) => {
-                        job.update_state(r.state);
-                        ResponseTemplate::new(StatusCode::OK)
-                    }
-                    (MockJobState::Running, MockJobState::Failed) => {
-                        job.update_state(r.state);
-                        ResponseTemplate::new(StatusCode::OK)
-                    }
-                    (MockJobState::Running, MockJobState::Running) => {
-                        job.update_state(r.state);
-                        ResponseTemplate::new(StatusCode::OK)
-                    }
-                    (current_state, _) if current_state != MockJobState::Running => {
-                        ResponseTemplate::new(StatusCode::FORBIDDEN)
-                    }
-                    _ => panic!("Invalid state change"),
-                };
+                assert!(
+                    matches!(
+                        r.state,
+                        MockJobState::Success | MockJobState::Failed | MockJobState::Running
+                    ),
+                    "Invalid state change from {} -> {}",
+                    job.state(),
+                    r.state,
+                );
 
-                r.append_header("Job-Status", &*job.state().to_string())
+                job.update_state(r.state);
+                ResponseTemplate::new(StatusCode::OK)
+                    .append_header("Job-Status", job.state().to_string())
             }
         } else {
             ResponseTemplate::new(StatusCode::NOT_FOUND)

gitlab-runner/src/client.rs

@@ -21,7 +21,6 @@ where
 }
 
 const GITLAB_TRACE_UPDATE_INTERVAL: &str = "X-GitLab-Trace-Update-Interval";
-const JOB_STATUS: &str = "Job-Status";
 
 #[derive(Debug, Default, Clone, Serialize)]
 struct FeaturesInfo {
@@ -250,8 +249,10 @@ impl JobResponse {
 pub enum Error {
     #[error("Unexpected reply code {0}")]
     UnexpectedStatus(StatusCode),
-    #[error("Job cancelled")]
-    JobCancelled,
+    // May occur if the job was cancelled, in which case the job token we're
+    // using gets invalidated.
+    #[error("Forbidden")]
+    Forbidden,
     #[error("Request failure {0}")]
     Request(#[from] reqwest::Error),
     #[error("Failed to write to destination {0}")]
@@ -336,13 +337,6 @@ impl Client {
         }
     }
 
-    fn check_for_job_cancellation(&self, response: &reqwest::Response) -> Result<(), Error> {
-        match response.headers().get(JOB_STATUS) {
-            Some(header) if header == "canceled" => Err(Error::JobCancelled),
-            _ => Ok(()),
-        }
-    }
-
     pub async fn update_job(
         &self,
         id: u64,
@@ -359,8 +353,6 @@ impl Client {
 
         let r = self.client.put(url).json(&update).send().await?;
 
-        self.check_for_job_cancellation(&r)?;
-
         let trace_update_interval = r
             .headers()
             .get(GITLAB_TRACE_UPDATE_INTERVAL)
@@ -369,6 +361,7 @@ impl Client {
             StatusCode::OK => Ok(JobUpdateReply {
                 trace_update_interval,
             }),
+            StatusCode::FORBIDDEN => Err(Error::Forbidden),
             _ => Err(Error::UnexpectedStatus(r.status())),
         }
     }
@@ -406,8 +399,6 @@ impl Client {
             .send()
             .await?;
 
-        self.check_for_job_cancellation(&r)?;
-
         let trace_update_interval = r
             .headers()
             .get(GITLAB_TRACE_UPDATE_INTERVAL)
@@ -417,6 +408,7 @@ impl Client {
             StatusCode::ACCEPTED => Ok(TraceReply {
                 trace_update_interval,
             }),
+            StatusCode::FORBIDDEN => Err(Error::Forbidden),
             _ => Err(Error::UnexpectedStatus(r.status())),
         }
     }

gitlab-runner/src/run.rs

@@ -248,7 +248,7 @@ impl Run {
             .await
         {
             Ok(_reply) => (),
-            Err(crate::client::Error::JobCancelled) => cancel_token.cancel(),
+            Err(crate::client::Error::Forbidden) => cancel_token.cancel(),
             Err(err) => warn!("Failed to update job status: {:?}", err),
         }
     }
@@ -276,7 +276,7 @@ impl Run {
                 self.log_offset += len;
                 reply.trace_update_interval
             }
-            Err(crate::client::Error::JobCancelled) => {
+            Err(crate::client::Error::Forbidden) => {
                 cancel_token.cancel();
                 None
             }